Adding a check on the grid/component consistency in the blueprints #2045
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
john-science
changed the title
john-science
reviewed
Feb 3, 2025
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
john-science
reviewed
Feb 3, 2025
|
Running integration tests, will update when they are done. |
|
The internal integration tests came back clean. I think this is ready for you to look at @john-science . |
Comment on lines
+188
to
+200
| idsInGrid = list(gridDesign.gridContents.values()) | ||
| if componentDesign.latticeIDs: | ||
| for latticeID in componentDesign.latticeIDs: | ||
| allLatticeIds.add(str(latticeID)) | ||
| # the user has given this component latticeIDs. check that | ||
| # each of the ids appears in the grid, otherwise | ||
| # their blueprints are probably wrong | ||
| if len([i for i in idsInGrid if i == str(latticeID)]) == 0: | ||
| raise ValueError( | ||
| f"latticeID {latticeID} in block blueprint '{self.name}' is expected " | ||
| "to be present in the associated block grid. " | ||
| "Check that the component's latticeIDs align with the block's grid." | ||
| ) |
There was a problem hiding this comment.
Just a thought.
This method is suddenly quite long.
Could we move this to a private helper method that just does the error handling? What do you think?
Comment on lines
+202
to
+210
| # for every id in grid, confirm that at least one component had it | ||
| if gridDesign: | ||
| idsInGrid = list(gridDesign.gridContents.values()) | ||
| for idInGrid in idsInGrid: | ||
| if str(idInGrid) not in allLatticeIds: | ||
| raise ValueError( | ||
| f"ID {idInGrid} in grid {gridDesign.name} is not in any components of block {self.name}. " | ||
| "All IDs in the grid must appear in at least one component." | ||
| ) |
There was a problem hiding this comment.
Just a thought.
This method is suddenly quite long.
Could we move this to a private helper method that just does the error handling? What do you think?
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?
Close #1400 .
This PR adds a check on the grids associated with a block to ensure that the components that are supposed to be in the grid (based on them having the
latticeIDsattribute) actually are, and in the correct number.Specifically, this PR enforces the following conditions:
If the conditions above are not met, it is very likely that the user has a bug in their blueprints that would otherwise be silently skipped.
Why is the change being made?
#1400 pointed out that there can be inconsistency here by pretty simple user errors. This has come up in my own work, and having ARMI be more robust to this kind of error is desirable.
Checklist
docfolder.pyproject.toml.