Cleanup group handling and make many functions into properties #17

gonuke · 2024-03-30T22:37:04Z

Improve group handling to ensure that duplicate groups with the same name are never created.

Keep a cache of Surface/Volume/Group ids to ensure that we don't reuse an ID already in use.

Makes many getter functions into properties and updates their calls.

Closes #12
Closes #15

pshriwise · 2024-04-02T14:04:36Z

dagmc/dagnav.py

+        self.used_ids[Surface._geom_dimension] = set(self.surfaces_by_id.keys())
+        self.used_ids[Volume._geom_dimension] = set(self.volumes_by_id.keys())
+        self.used_ids[Group._geom_dimension] = set(group.id for group in self.groups.values())


Small thing, but using the types themselves as the keys here will insulate us from any changes to properties of the types down the line:

Suggested change

self.used_ids[Surface._geom_dimension] = set(self.surfaces_by_id.keys())

self.used_ids[Volume._geom_dimension] = set(self.volumes_by_id.keys())

self.used_ids[Group._geom_dimension] = set(group.id for group in self.groups.values())

self.used_ids[Surface] = set(self.surfaces_by_id.keys())

self.used_ids[Volume] = set(self.volumes_by_id.keys())

self.used_ids[Group] = set(group.id for group in self.groups.values())

Other changes may be required elsewhere to comply with this change.

pshriwise · 2024-04-02T14:09:18Z

dagmc/dagnav.py

+
+    @property
+    def volumes_by_id(self):
+        return {v.id: v for v in self.volumes}

    @property
    def groups(self) -> Dict[str, Group]:


Along the lines of the discussion in #12, to be consistent with the volumes and surfaces properties this should probably return a list of Group instances and we can add a another method called groups_by_name to get the name --> Group instance mapping.

pshriwise · 2024-04-02T14:10:58Z

dagmc/dagnav.py

@@ -198,6 +214,12 @@ def id(self) -> int:
    @id.setter
    def id(self, i: int):
        """Set the DAGMC set's ID."""
+        if i in self.model.used_ids[self.geom_dimension]:


Suggested change

if i in self.model.used_ids[self.geom_dimension]:

if i in self.model.used_ids[type(self)]:

pshriwise · 2024-04-02T14:12:30Z

dagmc/dagnav.py

+        if i in self.model.used_ids[self.geom_dimension]:
+            raise ValueError(f'{self.category} ID {i} is already in use in this model.')
+        else:
+            self.model.used_ids[self.geom_dimension].discard(self.id)


Didn't know about this set method. Nice!

pshriwise · 2024-04-02T14:19:12Z

dagmc/dagnav.py

@@ -420,53 +446,54 @@ def groups(self) -> list[Group]:
        """Get list of groups containing this volume."""
        return [group for group in self.model.groups.values() if self in group]

+    def _material_group(self):
+        for group in self.groups:
+            if self in group and group.name.startswith("mat:"):


A couple of updates here.

The Volume.groups property already asserts that the volume is contained in the gruops, no need to recheck it here.

Double check me on this maybe, but I believe that our group name properties aren't required to be in a specific order, so as long as the keyword mat: appears somewhere in the group name it indicates a material assignment group.

Suggested change

if self in group and group.name.startswith("mat:"):

if 'mat:' in group.name:

pshriwise · 2024-04-02T14:23:03Z

dagmc/dagnav.py

-                return group.name[4:]
+        group = self._material_group
+        if group is not None:
+            return group.name[4:]


Along the lines of the comment above, we might require some more sophisticated parsing here if the material assignment doesn't come first in the group name. This is a pre-existing issue though, so I'll note it and we can clean it up in a subsequent PR.

pshriwise · 2024-04-02T14:23:34Z

dagmc/dagnav.py


    @property
    def volume(self):
        """Returns the volume of the volume"""
        volume = 0.0
-        for surface in self.get_surfaces().values():
+        for surface in self.surfaces:


A nice simplification as a result of the conversation in #12 :)

pshriwise · 2024-04-02T14:24:33Z

dagmc/dagnav.py

@@ -500,6 +527,9 @@ def name(self) -> Optional[str]:

    @name.setter
    def name(self, val: str):
+        if val.lower() in self.model.group_names:


The suggestion above about adding a DAGModel.groups_by_name method might change how this is approacched.

I left this method in since it is slightly cleaner than requesting by name and then taking only the keys.

pshriwise · 2024-04-02T14:27:24Z

dagmc/dagnav.py

+        or return an existing group if one exists."""
+
+        # return existing group if one exists with this name
+        if name is not None:


I'm not sure what to do with Group IDs. Part of me thinks we should notify if the group_id paramter is set and it doesn't match the returned Group.id if the group already exists. Functionally, the Group IDs really don't matter to DAGMC, but it still makes them complete in the eyes of MOAB. Thoughts?

I was on the fence about this, too. I'm happy to add it - it's pretty easy. I'll see what others have to say....

pshriwise

Thanks for tackling these updates @gonuke! Some minor thoughts on method purposes and line notes here.

pshriwise

Thank you for these design updates @gonuke!

gonuke added 7 commits March 30, 2024 16:50

update getters

75f4840

update calls to methods that are now poperties

e1b3f6a

update some more clals

3272199

fix more calls

6470336

track used ids

9c46631

update calls and start id safety tests

7d6a011

add to id safety tests

52fe027

gonuke mentioned this pull request Mar 31, 2024

Confirm safety of setting global ID #15

Closed

gonuke added 2 commits March 30, 2024 19:18

fix type

53a5081

cleanup group handling and ID caching

8ff7f99

gonuke changed the title ~~Make many functions into properties~~ Cleanup group handling and make many functions into properties Mar 31, 2024

gonuke added 3 commits March 31, 2024 14:48

comment typo

bbc4130

comment typos

54b5f75

Merge branch 'id_sets' of github.com:gonuke/pydagmc into id_sets

9c3423e

gonuke marked this pull request as ready for review March 31, 2024 19:51

gonuke requested a review from pshriwise March 31, 2024 19:51

pshriwise requested changes Apr 2, 2024

View reviewed changes

gonuke added 2 commits April 2, 2024 12:01

Updated interface per review

40d4d41

update tests for new interface

a275318

pshriwise approved these changes Apr 8, 2024

View reviewed changes

pshriwise merged commit 7ecdd45 into svalinn:main Apr 8, 2024
1 check passed

This was referenced Apr 8, 2024

Add DAGSet.create classmethod #13

Merged

Update readme to reflect recent API changes #20

Merged

pshriwise mentioned this pull request May 16, 2024

Slightly in consistent/unpredictable handling of multiple groups with same name #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup group handling and make many functions into properties #17

Cleanup group handling and make many functions into properties #17

gonuke commented Mar 30, 2024 •

edited

Loading

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

pshriwise Apr 2, 2024

gonuke Apr 2, 2024

pshriwise Apr 2, 2024

gonuke Apr 2, 2024

pshriwise left a comment

pshriwise left a comment

	if i in self.model.used_ids[self.geom_dimension]:
	if i in self.model.used_ids[type(self)]:

	if self in group and group.name.startswith("mat:"):
	if 'mat:' in group.name:

Cleanup group handling and make many functions into properties #17

Cleanup group handling and make many functions into properties #17

Conversation

gonuke commented Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise left a comment

Choose a reason for hiding this comment

pshriwise left a comment

Choose a reason for hiding this comment

gonuke commented Mar 30, 2024 •

edited

Loading