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

Piecewise grids for spiner EOS #330

Merged
merged 56 commits into from
Aug 13, 2024
Merged

Piecewise grids for spiner EOS #330

merged 56 commits into from
Aug 13, 2024

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Dec 8, 2023

PR Summary

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.

@Yurlungur Yurlungur changed the title [WIP] Piecewise grids for spiner EOS Piecewise grids for spiner EOS Aug 1, 2024
@Yurlungur
Copy link
Collaborator Author

@jhp-lanl @dholladay00 @chadmeyer this is ready for review. I think there's more one can do in this space, but IMO this is a nice start. In particular, energy and pressure near normal density are significantly better resolved with this method.

In anticipation of enabling spiner tables to load data from another eos, I move the Bounds object out of sesame2spiner into singularity-eos/base, and add tests to it.

I also found and fixed the issue @pdmullen found in riot regarding extrapolation. It was a bizarre interaction between fast logs and eospac in sesame2spiner. Now fixed.

@Yurlungur
Copy link
Collaborator Author

Thanks, @jhp-lanl !

I think the existing tests cover this, but please ensure that the tabular EOS changes are properly covered by our existing tests.

Yep already on that. The existing tests do cover this on re-git. So it's very important those get run before final merge.

@Yurlungur
Copy link
Collaborator Author

@dholladay00 please review. @chadmeyer please re-review or click approve depending.

@Yurlungur
Copy link
Collaborator Author

(tests now all passing on re-git.)


static void adjustForAnchor_(const Real min, Real &max, int &N,
const Real anchor_point) {
if (min < anchor_point && anchor_point < max) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the implications if this condition isn't met, would it make sense to error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the condition isn't met, it will just not adjust the number of points or the max. The table will still be valid, but there won't be a grid point exactly at the anchor point. I'd like sesame2spiner to basically always run... so I'd rather this not error out. But I'll add a warning.

@Yurlungur
Copy link
Collaborator Author

Thanks @dholladay00 I think I've addressed all your comments. Can you click approve if you agree? (I will not merge until post-change tests all pass on re-git)

Copy link
Collaborator

@chadmeyer chadmeyer left a comment

Choose a reason for hiding this comment

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

I think it looks good now for pushing. Good Job @Yurlungur!

@Yurlungur
Copy link
Collaborator Author

Thanks @chadmeyer @jhp-lanl @dholladay00 for your reviews! I now plan to merge as soon as the latest commit passes tests on re-git. Speak now if you have objections. :)

@Yurlungur Yurlungur merged commit 0b313cc into main Aug 13, 2024
5 checks passed
@Yurlungur Yurlungur deleted the jmm/hierarchical-grids branch August 13, 2024 18:09
#endif

constexpr int NGRIDS = 3;
using Bounds = singularity::table_utils::Bounds<NGRIDS>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Yurlungur since the Bounds class depends on spiner, can you wrap all of this in a SINGULARITY_USE_SPINER #ifdef?

I'm failing to build the tests when SINGULARITY_USE_SPINER is off.

I'll create an issue too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Performance Improving performance Robustness Ensures that existing features work as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants