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

added extra error checking on spherical mesh creation #2973

Merged
merged 5 commits into from
May 25, 2024

Conversation

chrwagne
Copy link
Contributor

@chrwagne chrwagne commented Apr 23, 2024

Description

Adding an additional check to ensure r_grid, phi_grid, and theta_grid values make sense when creating a spherical mesh, and raise a Value Error when they do not. This way a Value Error will be raised when a user creates attempts to create a spherical mesh with a r_grid, phi_grid, or theta_grid minimum that is greater than or equal to the maximum.

Fixes # 2933

#2933

Checklist

  • I have performed a self-review of my own code
  • I have followed the style guidelines for Python source files (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@chrwagne
Copy link
Contributor Author

@shimwell I have made these changes for my added error checks on the spherical mesh. Could you review them please?

openmc/checkvalue.py Outdated Show resolved Hide resolved
openmc/checkvalue.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

shimwell commented Apr 25, 2024

This looks super nice and concise. Very tidy PR. I could only spot a couple of extra f chars.

This looks ready to go in my view. I think we should merge this one in before #2977 then perhaps #2977 can be reshaped a bit to make use of the utility code here in the checkvalue.py

@shimwell
Copy link
Member

@paulromano would it be ok to merge this extra check in. Admittedly it will slow down the mesh creation a bit. Do you think the reduction in speed is worth the extra user friendliness? Would it be worth trying a numpy solution to speed things up a bit for the longer arrays?

Co-authored-by: Jonathan Shimwell <[email protected]>
@shimwell
Copy link
Member

Thanks for those changes, I've triggered the CI

@shimwell
Copy link
Member

CI has passed

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

I like the idea here but the implementation is not very efficient. I would recommend using numpy functionality rather than an explicit for loop. You can check for monotonicity of a list/array with:

np.all(np.diff(value) > 0.0)

@chrwagne
Copy link
Contributor Author

@shimwell @paulromano Thanks for the response and numpy changes applied!

@chrwagne chrwagne requested a review from paulromano April 27, 2024 00:59
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement, @chrwagne! Sorry for the delay on getting this merged.

@paulromano paulromano enabled auto-merge (squash) May 25, 2024 01:09
@paulromano paulromano merged commit 18cd81a into openmc-dev:develop May 25, 2024
17 checks passed
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
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.

3 participants