Add ability to create multiple groups of volumes at once #9
Conversation
dagmc/dagnav.py
Outdated
| if isinstance(volume, Volume): | ||
| group.add_set(volume) | ||
| else: | ||
| group.add_set(self.volumes[volume]) |
There was a problem hiding this comment.
This allows mix-and-match of volume IDs and Volume objects in a single list/dict... that could be confusing, but may it's OK?
There was a problem hiding this comment.
Sounds good - I'll add a test. I wasn't sure what the more popular use-case would be (I would probably just use IDs). This would allow for both without having to explicitly validate all the entries in the dictionary. I'm happy to add more input validation or change the data that is getting passed based on people's feedback (For instance, I'm not sure if the group ID is a good idea or bad idea. i.e. would we rather autogenerate group IDs similar to the IDManager in OpenMC?)
There was a problem hiding this comment.
I'm good with a mix of Volume objects and IDs. I think it's rare that a mix would be used, but if both are present the volume IDs still get validated by nature of a lookup on the DAGModel.volumes property.
I think auto-generation of Group IDs is a good idea. Maybe we can get to that soon.
dagmc/dagnav.py
Outdated
| group_map : dict | ||
| A dictionary mapping group names to a 2-tuple of (int, list) whose | ||
| first element is the group ID and the second element is a list of | ||
| volume IDs or Volume objects in the group. |
There was a problem hiding this comment.
Since groups contain either surfaces or volumes in a DAGMC model, what do you think about a generic function for generating a groups of surfaces. Probably less common that it would be used compared to the volume version.. idk. Thoughts?
There was a problem hiding this comment.
I agree and think the functionality to add multiple DagSets to a Group should probably live in the Group class. I just hadn't yet worked through exactly what the handles are and how to deal with that, but if you think an add_sets (rather than add_set) method or add/set_volumes/surfaces type methods would be valuable on the Group class I will go that route and use that functionality in the model level add_volume_groups method instead.
There was a problem hiding this comment.
Yeah let's start down that route, but if you hit any major road blocks don't spend too much time worrying about it at this point?
eepeterson
force-pushed
the
add_volume_groups
branch
from
d3a8663
to
a058fef
Compare
There was a problem hiding this comment.
Thanks @eepeterson - I just have one recommendation to expand testing.
| group = Group.create(self, name=group_name, group_id=group_id) | ||
|
|
||
| for dagset in dagsets: | ||
| if isinstance(dagset, DAGSet): |
There was a problem hiding this comment.
This case isn't tested. No reason to think it won't work, but may as well keep the test coverage up...
There was a problem hiding this comment.
good point - I added a case for volume and surfaces
There was a problem hiding this comment.
Thanks @eepeterson - LGTM. I'll let @pshriwise chime in and/or merge
This PR simply adds the capability to generate multiple groups of volumes in the
DAGModelwhich will help facilitate material assignment and MCNP UM model conversion.