Allow strictly finding material components #663
Conversation
MicahGale
added
alpha testing
|
resolved |
There was a problem hiding this comment.
Without running the code: the logic makes sense and the tests make sense.
I will give this function a test drive and confirm that everything works as expected before approving. In the meanwhile,
The type hinting for `nuclides is incorrect and one of the docstrings needs to be fixed. There are a few optional things to consider.
| Checks if this material contains any of the given nuclide. | ||
|
|
||
| A boolean "or" is used for this comparison. | ||
| That is this material must contain all nuclides at or above the given threshold |
There was a problem hiding this comment.
That this material must contain any nuclides at or above the given threshold!
| if mat.contains_all("U"): | ||
| pass | ||
|
|
||
| # try to find a uranium | ||
| .. note:: | ||
|
|
||
| For details on how to use the ``strict`` argument see the examples in: :func:`find`. | ||
|
|
||
| .. note:: | ||
|
|
||
| If a nuclide is in a material multiple times, and cumulatively exceeds the threshold, | ||
| but for each instance it appears it is below the threshold this method will return False. |
There was a problem hiding this comment.
Clarify behavior:
- If some nuclide appears multiple times, they are not summed when evaluating the threshold.
- But if an element is requested, the all isotopes of that element are summed [unless
strict]
Is this the intended behavior?
Is there any reason not to sum a nuclide that appears more than once? After all, we're just returning a bool, not a match.
| def _contains_arb( | ||
| self, | ||
| *nuclides: Union[Nuclide, Nucleus, Element, str, int], | ||
| bool_func: co.abc.Callable[co.abc.Iterable[bool]] = all, |
There was a problem hiding this comment.
I don't think bool_func should have a default.
| nuclides_search = {} | ||
| nuclei_search = {} | ||
| element_search = {} |
There was a problem hiding this comment.
Since we only ever use values(), these don't need to be dicts. We can actually get away with just a set.
The dicts make it easier to maintain and debug though, and the performance penalty is very minor.
|
|
||
| def get_containing_all( | ||
| self, | ||
| *nuclides: Union[ |
There was a problem hiding this comment.
*args are necessarily a tuple.
| self, | ||
| nuclide: Union[Nuclide, Nucleus, Element, str, int], | ||
| *args: Union[Nuclide, Nucleus, Element, str, int], | ||
| *nuclides: Union[Nuclide, Nucleus, Element, str, int], |
There was a problem hiding this comment.
*args are necessarily a tuple:
Tuple[Union[Nuclide, Nucleus, Element, str, int], ...]This type hint is long and frequently reused. Store it as a variable and reuse it.
| self, | ||
| nuclide: Union[ | ||
| *nuclides: Union[ |
There was a problem hiding this comment.
*args are necessarily a tuple
|
|
||
| def _contains_arb( | ||
| self, | ||
| *nuclides: Union[ |
There was a problem hiding this comment.
*args are necessarily a tuple
Pull Request Checklist for MontePy
Description
This is a feature implementation for
contains,get_containing, andfind. The topline is that there is a newstrictargument that switches from implicit to explicit matching. So withstrict=True"H"only matches1000and not1001.80cnor1000.80c. The other main thing was adding some feedback from @gonuke.This feedback splint
contains(andget_containing) to be:contains_allandcontains_any. This should be clearer to users, and be more useful. The main use case ofcontains_anymight be:Fixes #642
General Checklist
blackversion 25.Documentation Checklist
For infrastructure updates, I have updated the developer's guide.First-Time Contributor Checklist
pyproject.tomlif you wish to do so.Additional Notes for Reviewers
Ensure that:
📚 Documentation preview 📚: https://montepy--663.org.readthedocs.build/en/663/