Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confirm safety of setting global ID #15

Closed
gonuke opened this issue Mar 26, 2024 · 4 comments · Fixed by #17
Closed

Confirm safety of setting global ID #15

gonuke opened this issue Mar 26, 2024 · 4 comments · Fixed by #17
Assignees

Comments

@gonuke
Copy link
Member

gonuke commented Mar 26, 2024

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
Copy link
Member

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

@gonuke
Copy link
Member Author

gonuke commented Mar 31, 2024

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
Copy link
Member

pshriwise commented Mar 31, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants