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.

Sign up for GitHub

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

Jump to bottom

Implement reader for NuLib interaction tables #5902

Merged
merged 1 commit into from
May 24, 2024
Merged

Implement reader for NuLib interaction tables #5902

merged 1 commit into from
May 24, 2024

Conversation

ffoucart
Copy link
Contributor

@ffoucart ffoucart commented Apr 6, 2024

Proposed changes

Base implementation of a reader for NuLib tables of neutrino-matter interactions.

Upgrade instructions

NA

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

@ffoucart ffoucart force-pushed the NeutrinoInteractionTable branch 2 times, most recently from 4976990 to eeeb5d9 Compare April 9, 2024 14:59
@ffoucart ffoucart mentioned this pull request Apr 10, 2024
Merged
3 tasks
@ffoucart ffoucart force-pushed the NeutrinoInteractionTable branch 3 times, most recently from b5f2c05 to a20cef5 Compare April 18, 2024 20:18
@ffoucart
Copy link
Contributor Author

ffoucart commented May 7, 2024

Rebased against develop.

@ffoucart ffoucart mentioned this pull request May 9, 2024
Merged
3 tasks
@ffoucart
Copy link
Contributor Author

Added a second commit with a constructor for the interaction tables directly from vectors of interaction rates provided as input, instead of reading from a NuLib table. This allows for a simpler testing interface than having to generate fake tables.

@ffoucart ffoucart force-pushed the NeutrinoInteractionTable branch 2 times, most recently from 4b99218 to c518ce6 Compare May 13, 2024 20:46
nilsdeppe
nilsdeppe requested changes May 18, 2024
View reviewed changes
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

Looks great! I have a few minor suggestions. I think the biggest suggestion is using our existing multi-linear interpolation code. I think you can also squash the two commits into one :)

src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated Show resolved Hide resolved
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Show resolved Hide resolved
src/IO/H5/Helpers.cpp Outdated
@@ -134,6 +169,8 @@ Matrix vector_to_matrix(const std::vector<double>& raw_data,
}
return temp;
}

Copy link
Member

Choose a reason for hiding this comment

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

Remove added blank lines?

Copy link
Member

Choose a reason for hiding this comment

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

Ping on removing this blank line

src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.hpp Outdated Show resolved Hide resolved
@ffoucart
Copy link
Contributor Author

Not ready for second review yet; I still have to deal with interpolation and moving the physical constants to the proper header.

@ffoucart
Copy link
Contributor Author

@nilsdeppe Updated the code to use the existing multiD linear interpolator. As this changes a lot of the code (though not in a particularly difficult way), I kept the change as a separate commit. We can merge both commits once you're happy with the new version.

nilsdeppe
nilsdeppe reviewed May 23, 2024
View reviewed changes
Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

LGTM! Just one small ping. Please go ahead and squash and rebase :)

src/IO/H5/Helpers.cpp Outdated
@@ -134,6 +169,8 @@ Matrix vector_to_matrix(const std::vector<double>& raw_data,
}
return temp;
}

Copy link
Member

Choose a reason for hiding this comment

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

Ping on removing this blank line

@ffoucart
Copy link
Contributor Author

Removed blank line and squashed.

nilsdeppe
nilsdeppe previously approved these changes May 23, 2024
View reviewed changes
nilsdeppe
nilsdeppe reviewed May 23, 2024
View reviewed changes
src/Evolution/Particles/MonteCarlo/NeutrinoInteractionTable.cpp Outdated
number_of_vars;
idx = 2 * (NeutrinoSpecies * EnergyBins) + idx_emissivity;
table_data[idx] = buffer[ng][ns][ny][nt][nr] * length_unit;
std::clamp(table_data[idx], min_kappa, max_kappa);
Copy link
Member

Choose a reason for hiding this comment

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

aha! The macos failure is saying this should be:

table_data[idx] = std::clamp(table_data[idx], min_kappa, max_kappa);

@ffoucart
Copy link
Contributor Author

Fixed std::clamp and squashed.

@nilsdeppe nilsdeppe merged commit 9422b8f into sxs-collaboration:develop May 24, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilsdeppe nilsdeppe nilsdeppe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ffoucart @nilsdeppe