-
Notifications
You must be signed in to change notification settings - Fork 18
Support Spheres #877
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
base: develop
Are you sure you want to change the base?
Support Spheres #877
Conversation
|
I got the coverage up enough to pass, but think that |
MicahGale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be fun to merge into minor_rel_dev
Co-authored-by: Micah Gale <mgale@montepy.org>
Co-authored-by: Micah Gale <mgale@montepy.org>
MicahGale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes should be marked with the version the objects were added.
Also patch coverage is < 100%. I think you need to look into the validation testing some more.
| if self.location is None: | ||
| raise IllegalState(f"Surface: {self.number} does not have a location set.") | ||
|
|
||
| def find_duplicate_surfaces(self, surfaces, tolerance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a # pragma: no cover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why these IllegalState paths aren't getting tested in test_surfaces.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I mean for find_duplicate_surfaces, not for the IllegalState ones.
|
Ready for re-review. |
Changed in version 9.0: The versionadded directive was renamed to version-added. The previous name is retained as an alias.
MicahGale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta have more coverage.
| if self.location is None: | ||
| raise IllegalState(f"Surface: {self.number} does not have a location set.") | ||
|
|
||
| def find_duplicate_surfaces(self, surfaces, tolerance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I mean for find_duplicate_surfaces, not for the IllegalState ones.
| with pytest.raises(IllegalState): | ||
| surf.format_for_mcnp_input(vers) | ||
| # sphere on axis | ||
| surf = SphereOnAxis(number=5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these are getting caught by the parent class validate which requires a set surface_type. After the first validate call set surface_type.
Pull Request Checklist for MontePy
Description
Fixes #876 (issue number)
General Checklist
blackversion 25.LLM Disclosure
Were any large language models (LLM or "AI") used in to generate any of this code?
Documentation Checklist
Additional Notes for Reviewers
Ensure that:
Discussion
Spheres aren't candidates for periodic surfaces. I deleted duplicate surface checking related to that: please review carefully. There is a lot of copypasta for surface infrastructure that would be candidates for kicking up the
Surfaceclass or some other intermediate class.📚 Documentation preview 📚: https://montepy--877.org.readthedocs.build/en/877/