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

ENH: Stop mangling default figure file name if file exists #10864

Merged
merged 4 commits into from Mar 31, 2018

Conversation

mspacek
Copy link
Contributor

@mspacek mspacek commented Mar 22, 2018

Back in 2012, I added the feature that takes the title of the figure window and uses that as the default file name. Fairly recently, I noticed that the behaviour has changed. Now the default file name is prevented from ever possibly being identical to the name on disk, by appending a -1, -2, etc to the end of the base file name. This is absolutely daft in my opinion, and a bug, not a feature. There is no risk of accidentally overwriting an existing file, because the save dialog box, like any other, always first asks if you want to overwrite. I almost without fail do want to overwrite, which is why I hit save in the first place, but I have to constantly remove the useless -1 from the file name field every time I want to update my saved figures on disk.

This code restores the original behaviour.

I couldn't find any issues discussing the change from the original behaviour, and I don't know when it was introduced.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

If the file exists, the user is still prompted by the usual dialog box to decide whether or not to overwrite. The appended `-1`, `-2` are very annoying and not at all typical behaviour of a save dialog box.
@mspacek
Copy link
Contributor Author

mspacek commented Mar 22, 2018

Let me know if I should generate an issue to go with this as well. I'm not certain where the best place is for discussion.

@mspacek mspacek changed the title FIX: Stop mangling default figure file name if file exists ENH: Stop mangling default figure file name if file exists Mar 22, 2018
@anntzer
Copy link
Contributor

anntzer commented Mar 22, 2018

See also #9057. I don't agree with the turn the PR has taken (re: spaces in filenames), so I'm just mentioning it there but not championing it.

@tacaswell tacaswell added this to the v3.0 milestone Mar 22, 2018
@tacaswell
Copy link
Member

This came in via #4221 for 1.5 to address #3608 so the desired behavior is not universally agreed on.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 22, 2018

Wow, I really fell behind on updating MPL. That commit happened 2 years ago! Edit: Whoops, 3 years ago!

IMO, the feature request in the first post in #3608 is quite silly. The 1 in Figure_1 refers to the current figure number, and isn't intended as some kind of file versioning scheme. If you don't like the default name, change the figure window title with f.canvas.set_window_title(). It's very possible that option isn't widely known about though.

In the end, what was implemented wasn't even what was requested. Instead of creating consecutively named files Figure_1, Figure_2, Figure_3, ..., we ended up with the monster Figure_1, Figure_1-1, Figure_1-2, ...

In the spirit of compromise, I was going to propose that the behaviour be changed to something like:

if default_filename matches 'Figure_%d' % n:
    default_filename = 'Figure_%d' % (n+1)

But I feel so strongly about this that I think even that is a bad idea.

If you're saving multiple figures, you can't expect MPL to read your mind and know when to suggest a unique name, and when not to. So keep it simple, and don't try and mind read.

@tacaswell
Copy link
Member

If you don't like the default name, change the figure window title with f.canvas.set_window_title(). It's very possible that option isn't widely known about though.

This is definitely the case.

I like the compromise of the behavior depending on the figure name less than either behavior and increment n to not match the figure title even less.

In the current state we are not trying to mind read, the behavior of always suggesting a 'safe' name is straight forward and consistent with wget and most web-browsers.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 22, 2018

You're right, it isn't currently mind reading. It's worse. The decision was made, seemingly without much consideration, that no one ever wants to use the same name for an existing file. That's a massive assumption, not a mind read, and IMO, a terrible one. I go to the trouble of coming up with descriptive figure window titles like BL6_0348.tr1.r02.lfp.specgram() so that I end up with correspondingly descriptive file names. My reward is a -1 mindlessly appended to my file name by some logic that was designed as a crutch for those who never properly name their figures, and don't want to be bothered by a prompt to choose a different name if it already exists.

Now that I think about it a bit more, I'm willing to bet that for those who don't name their figures, the current behaviour creates more work for them in the long run. A lot of users probably end up with a whole swath of files with meaningless names. Instead of spending a little bit of time during the act of saving to choose a meaningful name or deciding whether to overwrite an existing file, that effort is put off over and over again, until it comes time to solve the inevitable problem: "Which one of these damned Figure_1-n.png files was the one I actually wanted to keep?" This is most definitely a bug, not a feature.

The suggested file name in the original implementation, and in this PR, is just as safe, if the measure of safety is "don't accidentally overwrite an existing file", thanks to the "do you want to overwrite?" dialog box. Safety is not an argument for the current behaviour.

MPL isn't a web browser, so let's not try and make it act like one. With MPL, the user has control over the default filename, and the skill level of the average MPL user is higher than that of the average web browser user. The average MPL user knows how to handle files, move them, delete them, and rename them. The average web browser user might not.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 22, 2018

It just occurred to me that it might be quite nice to add a "Window title" field to the figure options dialog box, maybe above the existing "Axes->Title" field. That would also give the hint that changing the figure window title is possible programmatically, the same way it is for the axes title.

@anntzer
Copy link
Contributor

anntzer commented Mar 23, 2018

fwiw I do like the proposed patch (or "unpatch", perhaps), and also agree that set_window_title could be better documented (not convinced about putting it in the UI though, as if the goal is to replace setting of the default filename that's basically redundant with, well, the save file dialog).

@jklymak
Copy link
Member

jklymak commented Mar 23, 2018

I always save via fig.savefig. I have a slight preference for suggesting a safe (unused) name.... Sorry but Figure_1.png is far more likely than MyFancyFigure.png, and we probably don't want people accidentally over-writing their figures from other sessions.

@efiring
Copy link
Member

efiring commented Mar 23, 2018

I agree with @mspacek that safety is not an issue because of the "do you want to overwrite..." dialog, so the business of adding '-1' etc. is not very helpful. One can imagine cases in which it saves some clicks and keystrokes; and others, such as @mspacek's, in which it costs clicks and keystrokes.
On balance I think this PR makes things simpler and cleaner, at no cost other than having to surprise some users with yet another behavior change.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 23, 2018

@jklymak, you're right, safety is an issue for fig.savefig, because that doesn't bring up a save dialog box, and doesn't warn you about overwriting. I really believe that automatically appending characters is a bad idea in general, and that it's better to force the user to make a decision on the spot what to do. IMO, if you're programmatically saving figures, you should know what you're doing, and you should know to check if you're overwriting an existing file if that's something you want to prevent. So I think it's fair to expect fig.savefig to be less safe than the save dialog box. However, maybe we could add something like a boolean overwrite=True kwarg to fig.savefig. If overwrite=False, raise an error if the filename already exists.

@ImportanceOfBeingErnest
Copy link
Member

Here is an anectotical description of why the current implementation may be advantageous.

I once had to save a couple of images from a plot, zooming and panning to different locations and saving the respective view. Doing so was quite easy, essentially (1) zoom/pan (2) ctrl+s (3) enter, restart with (1). This was so easy because I did not have to type in a new name for each frame.
Then, later naming the files coherently was a completely independent step, again easy (1) type name (2) press tab, restart with (1). In total very convenient.

Now if one wants to save the figure under the same name, with the current implementation, one would (1) ctrl+s (2) click the existing file in the file window that opens (3) click "save" or enter. So no need to type anything either.

In those cases above you can have one hand on the keyboard, one hand on the mouse.

However, if the proposed filename is always the same, one would need to type something new each time, resulting in either having to do monkey typing with one hand alone or switching between mouse and keyboard.

In general I do not have a strong personal opinion on this issue, but I wanted to share in how far it has been useful for me at least for one project in the past. Usually, the rate at which I need to save figures is slow enough that either implementation would work just fine.

@jklymak
Copy link
Member

jklymak commented Mar 23, 2018

I wasn’t suggesting the behaviour of savefig should change.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 23, 2018

@ImportanceOfBeingErnest

Then, later naming the files...

I'm pretty sure that for the majority of users, "later" means too late to remember what happened, and what the different figures represent. Or, if the plan was to immediately rename them after generating them, plans easily fall apart after a bit of distraction, and the rename never happens.

I feel very strongly that when prompting for a useful filename for the user's own future benefit, the user should not be allowed to say "don't bother me with this", or at least not more than once. The save dialog box is intentionally there to query the user. If you want to speed up the process of adding incremental characters to create a set of related figures, their names should be set programmatically.

@ImportanceOfBeingErnest
Copy link
Member

... for the user's own future benefit, the user should not be allowed to say "don't bother me with this"

If that was a valid argument, matplotlib would need to initialize any empty axes with x- and y- labels saying "Do not forget to label the graph", as I'm sure someone would claim that they feel like most users do not use meaningful axes labels.

The save dialog box is intentionally there to query the user.

According to this argument you could equally just leave the field empty to truely force people to type something in.

In total it is not really clear what would weight the desire not to type anything when saving the figure under the same name higher than the desire not to type anything when saving different figures.

I could imagine to add some customization for the interactive saving:

savefig.interactive.defaultname : somestring
      ## valid formatters {windowtitle}    # title of window
                          {increment}      # number to generate unique filename
		          {file}           # filename of python script if available
                          {figtitle}       # title of figure or first subplot

such that you may specify something like

savefig.interactive.defaultname : {windowtitle}

to get the behaviour desired in this RP, or

savefig.interactive.defaultname : {windowtitle}_{increment}

for the current behaviour, or

savefig.interactive.defaultname : {file}_{figtitle}_plotting_{increment}

to directly generate a useful filename according to the python scripts name (in case it is available) and the figure content.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 24, 2018

In total it is not really clear what would weight the desire not to type anything when saving the figure under the same name higher than the desire not to type anything when saving different figures.

You're confusing the issue with complicated language. I don't see how I could be any clearer. If I've gone to the trouble of explicitly naming the figure using set_window_title(), please don't go and change the default name on me in the save dialog box just because some people are unjustifiably afraid of accidentally overwriting an existing figure, or too lazy to come up with a unique file name if the default one already exists.

... for the user's own future benefit, the user should not be allowed to say "don't bother me with this"

If that was a valid argument, matplotlib would need to initialize any empty axes with x- and y- labels saying "Do not forget to label the graph", as I'm sure someone would claim that they feel like most users do not use meaningful axes labels.

That's a misquote. The full sentence was "the user should not be allowed to say "don't bother me with this", or at least not more than once." Axes labels are not files in a filesystem that have the potential to be overwritten, intentionally or not. Also, figures without axes labels are perfectly valid, but files without file names are not. Your analogy is false.

Certainly, making this configurable with rcParams would be great. Maybe someone is willing to invest time to do so. There would still of course be the issue of what the default setting should be. In the mean time, this to me is really a bug that needs fixing now, or more accurately, 3 years ago.

@efiring
Copy link
Member

efiring commented Mar 24, 2018

@mspacek, If you delete the test that is failing, I will approve this PR. Whether that will tip the balance remains to be seen. It will probably come down to a decision by @tacaswell.
My view:

  • The attempt to be "safer" by appending digits is not needed in the GUI case because of the "Do you want to overwrite" popup. (Is this true for all GUI backends?)
  • The one use case proposed by @ImportanceOfBeingErnest is probably a small fraction of actual uses, and even then is of minimal and debatable benefit; really, it is not so hard to type the digits into the filename if sequential digits, or any other modification of the filename, is desired.
  • The user of savefig should be responsible for entering a suitable filename.
  • The simplicity of the behavior proposed in this PR is an overall improvement.
  • This is a relatively minor issue, however, and not worth debating forever.

Eric

@ImportanceOfBeingErnest
Copy link
Member

How do we know what people want and need? Our feeling tells us that this is something 99% of users will not care too much about anyways. And we have one user who strongly needs the filename preserved (this PR) and we have one user who needed to have it incremented (the user of the last PR). The fact that noone has complained about it for 3 years now seems to suggest that people could live with how it is now. Certainly once this is changed back, someone will pull another request. So shouldn't there be a more durable solution implemented?

@anntzer
Copy link
Contributor

anntzer commented Mar 24, 2018

In general, I would approve and merge the PR (together with @efiring's approval, and unless @tacaswell really doesn't like it). However, the GTK3 backend currently *doesn't ask for overwrite confirmation, and that would need to be fixed first. Probably something involving https://developer.gnome.org/gtk3/stable/GtkFileChooser.html#gtk-file-chooser-set-do-overwrite-confirmation

All other backends do ask for confirmation. Interestingly, the macosx backend appears not to autorename the file, for a reason I haven't fully understood...

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

gtk3 backend needs confirmation checker.

@mspacek
Copy link
Contributor Author

mspacek commented Mar 27, 2018

Sorry for the delay.

@efiring, the failing test has been removed.

@anntzer, nice catch re GTK3. I added a single line which does the trick. Tested, works.

@ImportanceOfBeingErnest,

How do we know what people want and need?

By watching lots of novices and seeing what trips them up. It took me 3 years to complain because for about 2 of those, I was way out of date. For the last year, I put up with it because I wasn't generating heaps of figures, and let's face it, sometimes it takes time to make even the simplest of changes.

@dopplershift
Copy link
Contributor

"In the face of ambiguity, resist the temptation to guess."
Since it takes extra effort on our part to do the incrementing, I think our default behavior should be to not do the incrementing. Even more so since we have API calls in place that allow the user to set what they want--we shouldn't be overriding that by default and assume that we know better.

If we get complaints later, then I say add some RC parameters to control it.

@efiring
Copy link
Member

efiring commented Mar 27, 2018

Sorry, but one more thing: since it is a behavior change, it will need an entry in doc/users/next_whats_new.

@anntzer anntzer dismissed their stale review March 27, 2018 18:30

comments handled

@mspacek
Copy link
Contributor Author

mspacek commented Mar 30, 2018

Anything else I should do?

@tacaswell
Copy link
Member

How do we know what people want and need?

This is indeed one of the biggest challenges of this whole operation! Almost everything we do at the API / UI level is a judgement call.

I am over-all convinced that this is the right thing to-do (despite having also merged the PR that added this feature).

@tacaswell tacaswell merged commit f916aad into matplotlib:master Mar 31, 2018
@mspacek mspacek deleted the patch-1 branch March 31, 2018 17:49
@ImportanceOfBeingErnest
Copy link
Member

How do we know what people want and need?

In this Stackoverflow question they want to be able to set the default filename when the canvas is embedded in PyQt and hence does not have a figure manager with a set_window_title() method.

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

Successfully merging this pull request may close these issues.

None yet

7 participants