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

Allow the id.setter to take value of None #21

Merged
merged 8 commits into from
May 2, 2024
Merged

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Apr 8, 2024

If the id value passed to the id.setter is None automatically pick the next available ID

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment from me but then I think we can merge this in. Thanks @gonuke!

test/test_basic.py Outdated Show resolved Hide resolved
@gonuke gonuke marked this pull request as ready for review April 20, 2024 17:03
@@ -229,6 +236,7 @@ def test_coords(request, capfd):


def test_to_vtk(tmpdir_factory, request):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small thoughts/questions from me @gonuke. Thank you for this improvement!

dagmc/dagnav.py Outdated Show resolved Hide resolved
@gonuke
Copy link
Member Author

gonuke commented Apr 28, 2024

I moved the ID cleanup to DAGSet.delete() - but should that method also delete the DAGSet object?

@pshriwise
Copy link
Member

pshriwise commented Apr 30, 2024

I moved the ID cleanup to DAGSet.delete() - but should that method also delete the DAGSet object?

Can you elaborate on deleting the DAGSet object?

@gonuke
Copy link
Member Author

gonuke commented Apr 30, 2024

I moved the ID cleanup to DAGSet.delete() - but should that method also delete the DAGSet object?

Can you elaborate on deleting the DAGSet object?

As far as I know, the delete() method does not actually act as a destructor. So when we call delete() it acts on the MOAB DB, but there continues to be a DAGSet object on the python side that no longer points to anything on the MOAB side. Should we invoke a destructor on this object as part of delete() (if that's even possible?).

@pshriwise
Copy link
Member

As far as I know, the delete() method does not actually act as a destructor. So when we call delete() it acts on the MOAB DB, but there continues to be a DAGSet object on the python side that no longer points to anything on the MOAB side. Should we invoke a destructor on this object as part of delete() (if that's even possible?).

Ah, yeah I don't think it's possible to delete an object inside an object method. My understanding of invoking __del__ (via del) is that it will clean up any resources attached to an object before removing any references to that object in the current scope, meaning that if we call del self inside .delete to invoke the object destructor, a reference to the object will still exist in the scope from which the .delete method was called and the object won't be cleaned up.

It seems the best we can do is invalidate the object and ensure that it fails quickly and gracefully if used beyond the point that delete is called.

@gonuke
Copy link
Member Author

gonuke commented May 1, 2024

Ah, yeah I don't think it's possible to delete an object inside an object method.

I didn't think so.... but just wonder about what users will expect from the delete() method....

@pshriwise
Copy link
Member

I didn't think so.... but just wonder about what users will expect from the delete() method...

We're sort of limited by the language here, I'd say. So the best we can do is probably provide some additional documentation on what happens to the object at that point.... if we had documentation. For now, an expansion of the .delete() docstring would suffice I think.

@gonuke
Copy link
Member Author

gonuke commented May 2, 2024

I didn't think so.... but just wonder about what users will expect from the delete() method...

We're sort of limited by the language here, I'd say. So the best we can do is probably provide some additional documentation on what happens to the object at that point.... if we had documentation. For now, an expansion of the .delete() docstring would suffice I think.

I was also trying to think about whether delete() could have a better name, but couldn't come up with one.

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where we landed with this for now. Thank you @gonuke!

@pshriwise
Copy link
Member

I'll give this a bit in case @paulromano wants to have another look.

@paulromano
Copy link
Contributor

Looks good to me too! Thanks for pinging me on this @pshriwise

@pshriwise pshriwise merged commit 1783f5e into svalinn:main May 2, 2024
1 check passed
@pshriwise pshriwise mentioned this pull request May 2, 2024
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 this pull request may close these issues.

3 participants