Skip to content

MAINT: Remove ref on close#959

Merged
akaszynski merged 2 commits intopyvista:masterfrom
larsoner:close
Oct 23, 2020
Merged

MAINT: Remove ref on close#959
akaszynski merged 2 commits intopyvista:masterfrom
larsoner:close

Conversation

@larsoner
Copy link
Copy Markdown
Contributor

It seems strange to me that Plotter.__init__ adds a ref to this global _ALL_PLOTTERS but calling Plotter.close does not remove the reference. It really seems like it should. Otherwise you get memory accumulation even when closing your plotters, unless you call pyvista.close_all() (not a good solution because it closes all plotters, not just the one you might be trying to close) or modify pyvista._ALL_PLOTTERS (not a good solution -- private var).

This PR suggests that instead, let plotter.close take care of both keeping track of whether or not it's closed (by short-circuiting the .close operation if it has been done already) and keeping itself in _ALL_PLOTTERS.

@larsoner
Copy link
Copy Markdown
Contributor Author

It's also not clear to me why deep_clean is a separate method rather than just being part of close. Is this because people might expect to be able to do things with a given plotter once it's .closed?

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 21, 2020

Codecov Report

Merging #959 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
- Coverage   86.97%   86.92%   -0.06%     
==========================================
  Files          36       36              
  Lines        9060     9063       +3     
==========================================
- Hits         7880     7878       -2     
- Misses       1180     1185       +5     

@akaszynski
Copy link
Copy Markdown
Member

It's also not clear to me why deep_clean is a separate method rather than just being part of close. Is this because people might expect to be able to do things with a given plotter once it's .closed?

@banesullivan, you added that in at some point... what was the reasoning?

@larsoner
Copy link
Copy Markdown
Contributor Author

Okay CIs are happy, this is ready for review/merge from my end

Copy link
Copy Markdown
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up. I'd still like to know why we have deep_clean but we can consider removing/refactoring that in a later PR.

@akaszynski akaszynski merged commit 87ec4fa into pyvista:master Oct 23, 2020
@larsoner larsoner deleted the close branch October 23, 2020 15:06
GuillaumeFavelier added a commit to GuillaumeFavelier/pyvista that referenced this pull request Oct 24, 2020
akaszynski pushed a commit that referenced this pull request Oct 25, 2020
* TST: Make CIs happy

* Revert rest of #959
@banesullivan
Copy link
Copy Markdown
Member

I implemented this way to have it work well with sphinx-gallery. I think at this point, we've fixed the issues that lead to me having deep_clean be separate so I'd be in favor of refactoring this.

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