erdogant / pca

pca: A Python Package for Principal Component Analysis.
https://erdogant.github.io/pca
MIT License
286 stars 43 forks source link

What is the purpose of calling fig.set_visible(visible)? #46

Closed normanius closed 1 year ago

normanius commented 1 year ago

I like this package a lot, thanks for the work!

Often, users wish to adjust the appearance of plots before displaying it. When model.biplot() is called, one can suppress the call to plt.show() by setting the argument visible=False. However, this also sets fig.set_visible(False). In my Python / IPython environment, this has the side-effect that a later call to plt.show() will just show an empty figure, see below.

image

Setting the visible-status of the figure is not particularly user-friendly. I had to dig through this package's code to understand why the figure is not shown properly. For me, only the following works:

fig, ax = model.biplot(n_feat=4, visible=False)
ax.axis("scaled")  # Some user adjustment...
fig.set_visible(True) # Required, otherwise the figure will be empty!
fig.show()

The question: What's the purpose of the call to fig.set_visible()? I recommend omitting this in pca.py (I've counted two occurrences).

Another thing one might want to consider: don't call plt.show() in plotting routines! That's often left to the users in other plotting packages. Personally, I find it intrusive when a package determines the time when a window appears in my scripts 😉

erdogant commented 1 year ago

I am not sure whether I understand what the best way would be to fix this based on your suggestion. The reason for suppressing the figure is that sometimes you do need to see the figure but just need to save it to disk or so.

I can remove the fig.set_visible(visible) and use the plt.show(visible=visible) if it leads to no issues with Ipython. Is that what you are saying?

normanius commented 1 year ago

Does plt.show() take an argument visible?

If you want to call plt.show() for the user, I would do it like this:

if visible:
    plt.show()

But why calling plt.show() at all inside package methods?

In this seaborn example, one needs to explicitly call plt.show() to make the plot show. Here, the user controls when the figure is shown, if at all. The user can also control later, if a figure is shown or not by setting fig.set_visible(False)...

See this note from Michael Waskom on this matter.

See also this SO thread

erdogant commented 1 year ago

I see. Well once upon a time there was an issue (not pca but another) that plots were not visible when using the plot functionality. I then used plt.show() in the code to force it. Maybe this was a bug that is fixed. I can remove the plt.show because I do not see a reason to do it either. I will keep the fig.set_visible(visible) or do you suggest an alternative?

Yes, this note from Michael Waskom would likely have solved the issue back then.

normanius commented 1 year ago

I again would leave it to the user to call set_visible() on the returned figure object. In principle, you could remove visible from the API functions altogether. This is, however, a major change to the API, therefore you probably want to keep it, and just remove the plt.show().

That said, beginners may forget to call plt.show(), and feel frustrated that nothing happens. So one has to make sure that in examples plt.show() is called consistently.

erdogant commented 1 year ago

I created a new version where I removed plt.show()

pip install -U pca

erdogant commented 1 year ago

Closing this issue as it is fixed :)