Saving a PlotWindow object to a non-existent directory with a file prefix no longer works

[chummels]

Bug report

In the past, it was possible to save a SlicePlot or ProjectionPlot out to a non-existent directory with a custom prefix. but this no longer works. Now, you must create the directory first before saving there, or else you get a FileNotFoundError.

What does work is this.

import yt
ds = yt.testing.fake_amr_ds()
p = yt.SlicePlot(ds, "z", 'Density')
p.save("images/")

It works even when images does not yet exist, by first creating the images directory and then saving the file AMRGridData_Slice_z_Density.png into the images directory.

What does not work is this:

import yt
ds = yt.testing.fake_amr_ds()
p = yt.SlicePlot(ds, "z", 'Density')
p.save("images/test")

If you do not have an images directory created, it will fail with a FileNotFoundError. Previously, it would create an images directory, then output test_Slice_z_Density.png to it, similar to the non-prefixed example above.

The error that I now receive is:

Traceback (most recent call last):
  File "test_images.py", line 4, in <module>
    p.save("images/test")
  File "/Users/chummels/src/yt/yt/visualization/plot_container.py", line 76, in newfunc
    rv = f(*args, **kwargs)
  File "/Users/chummels/src/yt/yt/visualization/plot_container.py", line 564, in save
    names.append(v.save(name, mpl_kwargs))
  File "/Users/chummels/src/yt/yt/visualization/base_plot_types.py", line 149, in save
    canvas.print_figure(name, **mpl_kwargs)
  File "/Users/chummels/miniconda3/lib/python3.8/site-packages/matplotlib/backend_bases.py", line 2119, in print_figure
    result = print_method(
  File "/Users/chummels/miniconda3/lib/python3.8/site-packages/matplotlib/backends/backend_agg.py", line 535, in print_png
    with cbook.open_file_cm(filename_or_obj, "wb") as fh:
  File "/Users/chummels/miniconda3/lib/python3.8/contextlib.py", line 113, in __enter__
    return next(self.gen)
  File "/Users/chummels/miniconda3/lib/python3.8/site-packages/matplotlib/cbook/__init__.py", line 418, in open_file_cm
    fh, opened = to_filehandle(path_or_file, mode, True, encoding)
  File "/Users/chummels/miniconda3/lib/python3.8/site-packages/matplotlib/cbook/__init__.py", line 403, in to_filehandle
    fh = open(fname, flag, encoding=encoding)
FileNotFoundError: [Errno 2] No such file or directory: 'images/test_Slice_z_Density.png'

I guess maybe this is minor since people should be already create the directories to where they're saving files? But given that this used to work up until recently, and that it still works when you don't specify a prefix, maybe it's an easy fix and worth doing?

At the very least, if this is going to be left as is, I think we need to document it on the yt4 differences page. I tried to run a script that used to work and it broke and it took a while to track down what was going wrong.

[neutrinoceros]

This looks very much like a side effect of #2878 and #3176 where I attempted to dedupplicate a lot of code blocks that were handling this kind of stuff in very similar yet subtly different ways. I tried to break as little stuff as possible but honestly I'm not surprised something like this is broken now :/
I'm sorry I didn't document or adverstise it better. In my defense by the time it was considered for inclusion I had forgotten about how much it could affect downstream applications.

Anyway I agree this should be considered both a regression and a bug, and it should be fixed. Thank you for reporting !

[chummels]

Thanks for looking into this, @neutrinoceros ! No blame here, just trying to document where things go wrong for me, so hopefully it makes it easier for someone else too. :)

[neutrinoceros]

Actually I changed my mind about this. There are two reasons I see why the old behaviour may be deemed undesirable:

• it isn't consistent with matplotlib (pyplot.savefig never creates a missing dir, not even a single level)
• it is very hard to guarantee stability and consistency of this behaviour across yt's api and over time, since it would require adding this line

os.makedirs(os.path.dirname(file), exist_ok=True)

in many places, and there would always be a chance that we missed one. It's also arguably bad practice to create nested files implicitly, because the fact that the dirs are missing may be a sign that the application is actually faulty, and it's virtually impossible to correct for that from the outside. On the other hand, it's very easy for the user to obtain the behaviour you want if they want it (using the same line above), so I believe it should be the user's responsibility.

I'd even argue we shouldn't create the first dir level implicitly either, though I don't think it warrants a second behaviour change in yt 4.0.
If you agree with my reasoning, I suggest we close this and open a different issue (for 4.1) regarding my last point.

[chummels]

I disagree. I think that the auto-creation of a directory during save time is a very useful piece of functionality, and even if MPL doesn't do this, I think our user base has come to use it and expect it. I certainly use it all the time.

I guess I don't understand the difficulty with ensuring the codeline above is included in PlotWindow moving forward? There aren't that many places we call save.

I am certainly willing to be outvoted on this point, that we should remove the existing functionality that auto-creates a directory when one attempts to save a file to it, but I think it's useful and expected functionality. What do other people think?

[neutrinoceros]

There aren't that many places we call save.

About half a dozen currently, and I don't think they all share any one common ancestor (inheritance wise), which makes them harder to keep consistent whenever one of them change.

While I still think we should keep away from doing this kind of "magic" (creating data/files/dirs without explicit user consent), you're right that this is currently expectable from Yt since it's been with us for a while. What about reintroducing it on purpose, but with a deprecation warning ? (In which case the target removal version could be yt 4.2)

The auto dir generation could be implemented as a helper function, which would naturally give room for a deprecation cycle.

[matthewturk]

I think we should bring it up for discussion, before making a decision. @chummels shoot an email to yt-dev, pointing to this issue so folks know it's up for discussion?

[matthewturk]

One thing to note that @neutrinoceros and I just found is that plot_container.py's definition of save(), which will call individual PlotMPL objects to save(), does in fact call ensure_dir ... so it should be working, in some use cases. I'll dig in more as I am able.

[matthewturk]

Sorry, to be more clear, it will work if you allow it to set the full filename -- as in, if you call .save("something/") but maybe you knew that.

[neutrinoceros]

So @brittonsmith weighted in on this during today's triage and he prefers the previous behaviour (both cases in the original post should work). As for me, I like consistency more than I care about my previous argument, so I will fix this.

[neutrinoceros]

I forgot to deal with this last week end, I vow to fix it by next Sunday

[neutrinoceros]

I'm digging into this but I have to say the existing code seems very fragile and I can see at least two problems with this feature

Problem 1: what should happen here ?

p.save("img")

the current behaviour is that "img" is interpreted as a prefix to the image name unless an existing directory happens to be found with this name, in which case we save there instead, without a prefix.
This seems pretty ill-specified, hard to maintain and error-prone.

I think the best course of action here is to simplify things and make this only work as an image prefix. The saving directory can be specified using a trailing forward slash.

Problem 2:

Path separators / and \ are plateform-specific. Relying on them to match the platform we're on hurts portability.
We could support having / reinterpreted as \ on windows so that scripts using forward slashes only would be portable.
Right now this isn't done.

My point is that the existing code is pretty complicated already, I'm not sure I can fix this without breaking anything else or creating a pile of mud.

[neutrinoceros]

I wish this function wasn't doing so much magic at the same time. The more I touch it, the more I think it shouldn't do any.
Mainly, here are the two other features that make the function a mess:

• file extension can be specified from two different arguments (filename itself, + suffix)
• the core of the end filename is automatically created in every case: you can't define the full name from the user standpoint

Combined, these lead to weird situations from the user standpoint, where if one calls

p.save("myfile.png")

the actual output might be named myfile_predefined_filename.png.
I think automatic filenaming should be restricted to cases where the name isn't specified at all by the user.

[chummels]

Problem 1: what should happen here ?

p.save("img")

the current behaviour is that "img" is interpreted as a prefix to the image name unless an existing directory happens to be found with this name, in which case we save there instead, without a prefix.
This seems pretty ill-specified, hard to maintain and error-prone.

I think the best course of action here is to simplify things and make this only work as an image prefix. The saving directory can be specified using a trailing forward slash.

I agree with this course of action. I didn't realize that it was saving to a directory if that directory existed. I always thought that you would specify a prefix with a string (p.save('img')) and specify a directory with a string and a slash (p.save('img/')). I think that removing the "does a directory exist with the name img? then save there" functionality makes the most intuitive sense.

[chummels]

Problem 2:

Path separators / and \ are plateform-specific. Relying on them to match the platform we're on hurts portability.
We could support having / reinterpreted as \ on windows so that scripts using forward slashes only would be portable.
Right now this isn't done.

I see, so p.save('img/') will work for saving to the img/ directory, but not p.save('img\')? And will p.save('img/') work on a windows machine? I think if we can make sure that p.save('img/') works regardless of architecture, then that's probably good enough here.

[chummels]

Combined, these lead to weird situations from the user standpoint, where if one calls

p.save("myfile.png")

the actual output might be named myfile_predefined_filename.png.

I'm not finding this behavior to be true. When I run:

import yt
ds = yt.load_sample('IsolatedGalaxy')
yt.ProjectionPlot(ds, 'x', ('gas', 'density')).save('myfile.png')

I see myfile.png in my directory, not myfile_predefined_filename.png.

[neutrinoceros]

I think if we can make sure that p.save('img/') works regardless of architecture, then that's probably good enough here.

that's what I've been doing with yt.load_sample, I agree it's likely the best solution to support this everywhere windows users can safely copy-paste sample code from the documentation, and script can always be written in an OS-agnostic way :)

[neutrinoceros]

I see myfile.png in my directory, not myfile_predefined_filename.png.

I was commenting from memory. I'm happy that it turns out this was wrong, but can you guess what happens if you write this instead ?

yt.ProjectionPlot(ds, 'x', ('gas', 'density')).save('myfile.png', suffix="pdf")

I know I can't, and I've been working on that code already :/