openmmtools
openmmtools copied to clipboard
remove del operator on PhaseAnalyzer
fixes #633
the __del__
doesn't do anything except delete attributes, so isn't necessary
I haven't looked at this bit of code lately, maybe @ijpulidos could check, but __del__
does call self.clear()
, which does some cache clearing stuff
I did a quick scan, there's no explicit .close()
or similar calls, it's just removing references from the _cache
dict, which is what Python garbage collection should also be doing. I didn't have time to go through the git history to see if there used to be a file handle held....
@richardjgowers thanks for reporting the issue and contributing the possible solution.
I tend to agree with your remarks, the Python garbage collector should already be doing this. Nevertheless, I would like to have a reproducible example/snippet where the issue appears, it doesn't have to be a simple one, just something I can use to reproduce the issue and go from there. If you have one I can use that would be very helpful. Thanks!
We should probably catch the AttributeError
in _invalidate_cache_values
since (from https://stackoverflow.com/a/1481512) mentions
However, there are valid use cases for del: e.g. if an object X references Y and also keeps a copy of Y reference in a global cache (cache['X -> Y'] = Y) then it would be polite for X.del to also delete the cache entry.
Which looks like what we are doing here.
Also from the docs it sounds like __del__
" [is] called when the instance is about to be destroyed" so we really just be using context mangers if cleanup is important with an __exit__
(or finally
block) since it might not get called until the intrepter exits.
Codecov Report
Merging #634 (229053f) into main (6cef59b) will decrease coverage by
0.01%
. The diff coverage isn/a
.
Aha ok. For context, I think I was getting this error when I was trying to create a PhazeAnalyzer
and it was failing in __init__
which is why you end up trying to delete an attribute that should probably always exist.