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

Consistency Check for Indexed Outputs (e.g., Shell_Slices, Meridional_Slices) #574

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

feathern
Copy link
Contributor

@feathern feathern commented Aug 19, 2024

This PR implements some consistency/sanity checks when Rayleigh interprets the values of variables such as shellslice_levels, meridional_indices etc. Previously, Rayleigh would accept values that fell outside of the grid bounds, which could lead to crashes. With this addition, indicial values that fall outside of the grid bounds are filtered out. If unallowed values are specified for the normalized indices (e.g., shell_slice_levels_nrm), those should also be filtered out by these modifications to the code logic.

This addresses the cause of the 2nd crash reported in issue #570.

Copy link
Contributor

@tukss tukss left a comment

Choose a reason for hiding this comment

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

Thank you for that. I have some small stylistic changes. Feel free to take them or not.

I have tested that it caps subranges as expected. It doesn't actually do anything when we specify single indices outside the valid range, correct? Like setting meridional_indices = 180 in a simulation with ntheta = 16.

src/IO/Spherical_IO.F90 Outdated Show resolved Hide resolved
src/IO/Spherical_IO.F90 Outdated Show resolved Hide resolved
src/IO/Spherical_IO.F90 Outdated Show resolved Hide resolved
@feathern
Copy link
Contributor Author

Thanks for reviewing this. That's correct, any indices specified outside the allowed ranges are simply ignored. I can probably add a warning message in a future PR if you want. I'm fine with all of your suggested changes and just pushed an updated version. Also, the call to that routine that was commented out was from an earlier version -- there is no longer intended to be a routine named enforce_bounds. Thanks for catching that.

Copy link
Contributor

@tukss tukss 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.

@feathern feathern merged commit 919960a into geodynamics:main Aug 22, 2024
7 checks passed
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.

2 participants