Repository navigation
Fix Resetting of Auto IDs When Generating MGXS - #3437
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that after loading materials and geometry from XML, any new objects receive IDs higher than existing ones to avoid conflicts. Key changes include:
- Introduction of
update_auto_idsto sync ID counters based on loaded IDs. - Removal of
reset_auto_idscalls from MGXS generation routines. - Enhanced handling of unnamed or whitespace-only material names.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openmc/model/model.py | Removed obsolete reset_auto_ids calls from MGXS generation methods |
| openmc/mixin.py | Added update_auto_ids to update next_id for all IDManagerMixin subclasses |
| openmc/material.py | Imported and invoked update_auto_ids in Materials.from_xml_element |
| openmc/geometry.py | Imported and invoked update_auto_ids in get_universe to sync IDs after geometry load |
Comments suppressed due to low confidence (5)
openmc/model/model.py:1554
- After loading a pre-existing model, new MGXS routines may still generate conflicting IDs; consider calling
update_auto_ids()before constructing the newModelto ensure next_id is set above any loaded IDs.
model = openmc.Model()
openmc/model/model.py:1751
- The removal of
reset_auto_ids()here requires an explicit call toupdate_auto_ids()to avoid ID collisions when adding new objects in the stochastic slab MGXS generation.
model = openmc.Model()
openmc/model/model.py:1862
- In the material-wise MGXS method, consider invoking
update_auto_ids()after deep-copyingselfso that new tallies and cells receive non-conflicting IDs.
model = copy.deepcopy(self)
openmc/mixin.py:120
- Add unit tests for
update_auto_ids()to verify thatnext_idis correctly set above the maximum ofused_idsin typical XML-load scenarios.
def update_auto_ids():
openmc/mixin.py:120
- [nitpick] The name
update_auto_idsmight be ambiguous; consider renaming tosync_next_idsorrefresh_auto_idsto clearly differentiate it from the oldreset_auto_idsbehavior.
def update_auto_ids():
paulromano
left a comment
paulromano
left a comment
There was a problem hiding this comment.
@jtramm Even though next_id will indeed still be at 1 assuming the model you loaded didn't use ID 1, this shouldn't be a problem because when new objects are created, the id setter from IDManagerMixin checks whether next_id is already within the set of used_ids and increments it until it finds one that isn't used. So, to my understanding there shouldn't be any conflicts. Are you finding otherwise?
|
Yes, I was getting conflicts in the JET model which motivated this. However, I'm now wondering if the other change I made in this PR to remove the calls to: openmc.reset_auto_ids()in |
jtramm
changed the title
|
Indeed -- the problem was being caused in |
Description
Currently if you load a model from an xml file, any subsequent surfaces you add to the model will start from index 1, often conflicting with existing surfaces. This PR adds a check after loading materials and geometry objects from XML to set the ID to one above the largest currently in the model.
This use case comes up with random ray, where we might want to load an existing .xml model but then place it inside a smaller box with vacuum boundaries, which involves the creation of additional cells and surfaces.
This PR also adds a more careful treatment of checking for unnamed materials.
Checklist