openmmtools icon indicating copy to clipboard operation
openmmtools copied to clipboard

remove del operator on PhaseAnalyzer

Open richardjgowers opened this issue 2 years ago • 6 comments

fixes #633

the __del__ doesn't do anything except delete attributes, so isn't necessary

richardjgowers avatar Nov 25 '22 15:11 richardjgowers

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

mikemhenry avatar Nov 28 '22 19:11 mikemhenry

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 avatar Nov 28 '22 19:11 richardjgowers

@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!

ijpulidos avatar Nov 28 '22 20:11 ijpulidos

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.

mikemhenry avatar Jun 14 '23 16:06 mikemhenry

Codecov Report

Merging #634 (229053f) into main (6cef59b) will decrease coverage by 0.01%. The diff coverage is n/a.

codecov[bot] avatar Jun 14 '23 16:06 codecov[bot]

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.

richardjgowers avatar Jun 14 '23 16:06 richardjgowers