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

renamercParams['savefig.directory'] to rcParams['gui.save.directory'] #20790

Open
astromancer opened this issue Aug 4, 2021 · 12 comments
Open

renamercParams['savefig.directory'] to rcParams['gui.save.directory'] #20790

astromancer opened this issue Aug 4, 2021 · 12 comments

Comments

@astromancer
Copy link
Contributor

@astromancer astromancer commented Aug 4, 2021

Bug summary

Figures saved via the savefig method are not saved relative to the rcParams['savefig.directory'] path. Looking at the source code, it seems that this path is only prepended to the filename if the save request came via the interactive toolbar. Not sure if this is done intentionally, but it seems to me that the savefig.directory should always be respected.

In the example below I use the home directory ~, but the same behaviour occurs for any other existing directory.

Code for reproduction

from pathlib import Path
from matplotlib import rc, pyplot as plt


# All figures to go into home
rc('savefig', directory='~')

fig, ax = plt.subplots()
fig.savefig('test.png')

(Path.home() / 'test.png').exists() # False => Not saved in rcParams['savefig.directory']
(Path('.') / 'test.png').exists() # True => Saved in current working directory

Actual outcome

The figure is saved in the local working directory.

Expected outcome

If rcParams['savefig.directory'] has been set, and savefig is given a filename (not a full path), the figure should be saved relative to savefig.directory.

Operating system

Ubuntu

Matplotlib Version

3.3.4

Matplotlib Backend

Qt5Agg

Python version

3.8.10

Jupyter version

NA

Other libraries

No response

Installation

pip

Conda channel

No response

@anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 4, 2021

This behavior is intentional and will almost certainly not change. Perhaps it could be better documented, though?

@jklymak
Copy link
Contributor

@jklymak jklymak commented Aug 4, 2021

I dunno, docs are pretty explicit:

# default directory in savefig dialog box,
# leave empty to always use current working directory

I'd vote to close this....

@astromancer
Copy link
Contributor Author

@astromancer astromancer commented Aug 4, 2021

OK, If it's a wontfix I think it will be worth mentioning in the documentation for savefig that this is a potential gotcha.

@jklymak
Copy link
Contributor

@jklymak jklymak commented Aug 4, 2021

I am not sure how the rcParam could be any more explicit, though I agree it was poorly named. I guess we could deprecate the rcParam and rename it guisavefig....

@astromancer
Copy link
Contributor Author

@astromancer astromancer commented Aug 5, 2021

Well, I have to confess that I didn't come here after reading the rcParam docs - I spotted the key and tried to use it intuitively like

rc('savefig', dpi=600, transparent=True, directory='/path/to/folder')
...
fig.savefig('file.png')

Which works for the dpi and transparency.

Personally it would be useful to me to be able to set the save directory programmatically. For example, when running a pipeline that produces many figures, its convenient to set the output directory at startup and then subsequently only having to use filenames instead of the full path when saving figures.

@anntzer

This behavior is intentional and will almost certainly not change.

Just curious why you see this is as the case? I'm not proposing to change the default behaviour where rcParam['savefig.directory'] = '' - which is what most people use i think. The only change would be when the user has set rcParam['savefig.directory'] and then provided only a filename to a savefig call, the file will be saved in the rcParam directory instead of the current working directory. Calls the come via GUI interactions would be unchanged.

rcParam['savefig.directory'] '' ~ '' ~
fig.savefig(fname=...) x.png x.png /y/x.png /y/x.png
current outcome ./x.png ./x.png /y/x.png /y/x.png
proposed ./x.png ~/x.png /y/x.png /y/x.png

What will this require? Inside savefig: If rcParam['savefig.directory']is non empty and fname is path-like and not

some backend-dependent object such as matplotlib.backends.backend_pdf.PdfPages

then parse it and only if it's not absolute, prepend the rc directory. I think this will make the API more consistent. Are there people that set rcParam['savefig.directory'] and still expect savefig to save in the local directory? I think not that many, but i could be wrong.

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 6, 2021

savefig.directory is also used as a runtime location, to store the last used directory for interactive saves, if not empty. Having this suddenly start affecting non-interactive fig.savefig would be confusing.

Additionally, savefig works like open or any other IO function, defaulting to the current working directory. If it started obeying some random configuration, that would quite likely break a bunch of scripts and existing packages.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented Aug 9, 2021

I very much agree with @QuLogic that savefig departing from the behavior of open would be a major API change and is a non-starter. That said, I also very much agree with @astromancer that the rcparam name is very confusing and suggestive of the proposed behavior.

I think the path forward here is to rename savefig.directory to interactive_savefig.last_used_directory or move this out of rcparams all together. I could also see the case for moving to a bool rc['interactive_save_fig.sticky_last_path'] and then expecting the GUI backends to stash the last used directory someplace at their module level rather than rcparams.


This came in via #1685 if people want to see the original discussion.

@timhoffm
Copy link
Member

@timhoffm timhoffm commented Aug 9, 2021

+1 for renaming. How about gui.save.directory?

I think save[_]fig in the name is confusing because that will always be associated with the savefig method.

I don‘t see a strong case for switching to bool. Giving directory explicitly allows for a special default place to sore images to. I don‘t think we should strip that feature (and I personally use that sometimes). If anything sticky_last_path should be an additional option. But as long as nobody asks for it, I wouldn’t go there.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented Aug 9, 2021

I agree we should not drop the feature.

The strongest argument for switching to bool is that I think this on one of two rcparam that we expect to be changed by the library code under normal operations (the other is 'backend' which we only change once from the secret sentinel to the selected backend so the user can never (without cheating) observe it change) so it is just a bit funny / defies the expectation that "rcparams is a place for users to pass global default parameters to Matplotlib" (by being "a place for Matplotlib to stash some internal state").

@timhoffm
Copy link
Member

@timhoffm timhoffm commented Aug 11, 2021

I'd say that "rcparams is the space to store (global) config state".

It's not internal (and also gui.save.directory doesn't have to be internal). And yes, users can manipulate that state, with matplotlibrc config files as well as in a local context.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented Aug 11, 2021

The distinction I was trying to draw is between the user change the state vs us changing the state.

I'm not going to push this farther and am happy with gui.save.directory

@tacaswell tacaswell changed the title [Bug]: Figure.savefig does not respect rcParams['savefig.directory'] renamercParams['savefig.directory'] to `rcParams['gui.save.directory'] Aug 11, 2021
@tacaswell tacaswell changed the title renamercParams['savefig.directory'] to `rcParams['gui.save.directory'] renamercParams['savefig.directory'] to rcParams['gui.save.directory'] Aug 11, 2021
@tacaswell tacaswell added this to the v3.6.0 milestone Aug 11, 2021
@astromancer
Copy link
Contributor Author

@astromancer astromancer commented Aug 12, 2021

Thanks for the insights @QuLogic and @tacaswell. I'll try submit a PR when I have the bandwidth available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants