Relax Error Check for 1D cylindrical collection #1347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@tjayasa FYI, I think your work was one of the motivations for this PR? |
I'd sugget putting a comment in the code saying as much, for future readers. |
keckler
approved these changes
Jul 25, 2023
Comment on lines
+484
to
+486
| diffNucs = theseNucs.symmetric_difference(thoseNucs).intersection( | ||
| consistentNucs | ||
| ) |
There was a problem hiding this comment.
This check is, IMO, pretty opaque and could use a comment. It works, but is one of those "elegant but unreadable" pieces of code.
Co-authored-by: Chris Keckler <kecklerct@gmail.com>
mgjarrett
commented
Jul 25, 2023
mgjarrett
commented
Jul 25, 2023
mgjarrett
added a commit
to mgjarrett/armi
that referenced
this pull request
Aug 24, 2023
Co-authored-by: Chris Keckler <kecklerct@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the change?
The
CylindricalComponentsAverageBlockCollectionwas recently added: #1238This collection is in some ways modeled after the
SlabComponentsAverageBlockCollection, which is much older. The_checkComponentConsistencyconcept is similar, where each analogous component of each block in the collection is checked for consistency before homogenizing multiple blocks into one representative block. However, the new implementation checked that two components being homogenized into one have exactly the same nuclides. The old implementation just checked key nuclides like U-235, Pu-239, Fe-56, and Na-23 to verify that components were similiar; i.e., structure being merged with structure (Fe-56), fuel with fuel (U-235/Pu-239), coolant with coolant (Na-23).Why is the change being made?
The new implementation was excessively picky about what can be homogenized together. Two similar assembly types where one has some extra trace isotopes in the cladding would throw a
ValueErrorinstead of just graciously being homogenized together.This PR revises the implementation to make it functionally equivalent to the similar, older method.
Note: using Fe-56 as a proxy for structure and Na-23 for coolant is undesirably SFR-centric, but I'm not going to address that in this PR.
Checklist
doc/release/0.X.rst) are up-to-date with any important changes.docfolder.setup.py.