search

svalinn / pydagmc

A convenience interface for examining and reassigning metadata in DAGMC models with PyMOAB
MIT License
12 stars 8 forks source link

Confirm safety of setting global ID #15

Closed gonuke closed 7 months ago

gonuke commented 7 months ago

The DAGSet class has a setter for the id which changes the value of the global_id tag in the MOAB model.

I wonder if we should be testing for the existence of a given ID in the ID space already to avoid creating multiple objects with the same global id?

pshriwise commented 7 months ago

I think we should, yeah. We can get the set of used IDs when creating the DAGModel object to track this.

gonuke commented 7 months ago

It seems that it is intentional to allow a new group to be made with the same name as an old group, and there is a tests that explicitly tests that this works correctly.

I'm curious what the use case is for this? It seems that it might be dangerous?

17 currently calls this a failure but happy to revert

pshriwise commented 7 months ago

Separating issues of concern here: global IDs and group names.

Global IDS

For avoiding duplicate global IDs, to me it makes sense to maintain a set of used IDs on the DAGModel supporting the various DAGSet instances so that if a set ID is changed, it isn't already occupied. We should maintain a set for each topological dimension to handle this of course.

One use case I can see for changing the IDs of the sets is to avoid clashes with the ID space of CSG entities when generating a hybrid CSG/DAGMC model. There's also precedent for this if PyDAGMC is to be used to support model creation. I believe @paulromano has an interest in adding such capabilities.

Group Names

I'm not sure of a use case for groups with duplicate names either, but I'm not sure that it's dangerous. From the look of the DagMC::parse_properties method, if there's already an entry for a property from a previous group set, then it will be added the property tagmap that is being generated.

https://github.com/svalinn/DAGMC/blob/7e1cdb638b59ba22fd6ff0b09e94d8f658a1eadf/src/dagmc/DagMC.cpp#L1174

Similar to the set IDs, I think we can maintain an additional set for groups to ensure that a name isn't already occupied. If creating a new group and the name is already present, we can return the existing group from the Group.create method (perhaps with a warning). Note to whoever implements this, my future self included: I'd vote that we use lowercase string comparison for matching.

pshriwise commented 7 months ago

Oh I see that you've self-assigned here, @gonuke, so maybe that note at the end is really just for you 😁