Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fork/alexwenym #38

Merged
merged 28 commits into from
Jan 10, 2024
Merged

Fork/alexwenym #38

merged 28 commits into from
Jan 10, 2024

Conversation

nickkamp1
Copy link
Collaborator

Methods for sampling tabulated distribution using the inverse CDF method. Fixes a bug with the ATLAS example.

@austinschneider
Copy link
Collaborator

There's a whole lot of changes in this branch. I think most of them are good, but it is worth going through them all invidually before we merge it.

Copy link
Collaborator

@austinschneider austinschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few minor changes that need to be made before we merge this. Mainly it's removing some comments, a few stray printouts, and deciding if we want to keep some of the files added to resources. There is an issue with how the random numbers are created for the inverse cdf sampling though.

projects/crosssections/CMakeLists.txt Outdated Show resolved Hide resolved
projects/detector/private/EarthModel.cxx Outdated Show resolved Hide resolved
projects/detector/private/EarthModel.cxx Show resolved Hide resolved
projects/detector/private/EarthModel.cxx Outdated Show resolved Hide resolved
projects/detector/private/EarthModel.cxx Outdated Show resolved Hide resolved
resources/ATLAS_SNe_Injection/inject_nu_ATLAS.py Outdated Show resolved Hide resolved
resources/earthparams/densities/PREM_ATLAS-earthcenter.dat Outdated Show resolved Hide resolved
resources/earthparams/densities/PREM_ATLAS_backup.dat Outdated Show resolved Hide resolved
resources/DipoleInjection/makeerror.log Outdated Show resolved Hide resolved
Copy link
Collaborator

@austinschneider austinschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar changes needed as previous review

Copy link
Collaborator

@austinschneider austinschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly whitespace

prob_density /= (M_PI * radius * radius); // (m^-1 * m^-2) -> m^-3

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

projects/injection/private/TreeWeighter.cxx Outdated Show resolved Hide resolved
@@ -263,7 +265,7 @@ double LeptonProcessWeighter::NormalizedPositionProbability(std::pair<LI::math::
record.interaction_vertex[0],
record.interaction_vertex[1],
record.interaction_vertex[2]);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Collaborator

@austinschneider austinschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@austinschneider austinschneider merged commit 46841b0 into main Jan 10, 2024
0 of 14 checks passed
@austinschneider austinschneider deleted the fork/alexwenym branch January 10, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants