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

Hotfix image filename validation #3176

Merged
merged 4 commits into from
Apr 10, 2021

Conversation

neutrinoceros
Copy link
Member

PR Summary

The following script has two bugs

from yt.testing import fake_amr_ds
from yt import SlicePlot

ds = fake_amr_ds()
p = SlicePlot(ds, "z", "Density")
p.save("1.2.3")

1 - the logger contains a stupid entry (not meaning offense to anyone but myself, I authored it)

yt : [WARNING  ] ... Ignoring supplied suffix '.png' in favour of '.png'

2 - the resulting file is named 1.2.png, while it should clearly be 1.2.3.png (discovered this while trying to embed matplotlib version numbers in file names...)

This fixes both these problems and provides better testing (yay, pytest !) for the culprit function.

@neutrinoceros neutrinoceros added bug api-consistency naming conventions, code deduplication, informative error messages, code smells... labels Apr 8, 2021
@neutrinoceros neutrinoceros force-pushed the hotfix_image_filename_validation branch 3 times, most recently from 149aa7b to 6cad08e Compare April 8, 2021 20:07
@cphyc cphyc closed this Apr 8, 2021
@cphyc cphyc reopened this Apr 8, 2021
Copy link
Member

@cphyc cphyc left a comment

Choose a reason for hiding this comment

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

LGTM!

(sorry for closing/reopening, that was a mistake!)

yt/visualization/_commons.py Outdated Show resolved Hide resolved

return f"{name}{suffix}"
if suffix is None:
suffix = ".png"
Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Suggested change
suffix = ".png"
suffix = ytcfg.get("yt", "default_image_format")

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ambivalent. I don't like having a parameter with "default" in its name, makes it a bit hard to know what we're talking about. That being said I like the idea of parametrising a "preferred" output format (though that name isn't perfect either). I'd like to think about this a bit more and maybe do it in a separate PR.

@neutrinoceros neutrinoceros force-pushed the hotfix_image_filename_validation branch from 6cad08e to 365610a Compare April 9, 2021 07:34
@cphyc cphyc merged commit f4e0f94 into yt-project:main Apr 10, 2021
@neutrinoceros neutrinoceros deleted the hotfix_image_filename_validation branch April 10, 2021 14:32
@neutrinoceros neutrinoceros added UX user-experience and removed api-consistency naming conventions, code deduplication, informative error messages, code smells... labels May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug UX user-experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants