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: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format. #3410

Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jul 4, 2021

PR Summary

Drop a hard-coded list of supported file formats and discover them at runtime via Matplotlib instead.
Incidentally, this adds support for saving to .svgz, which is compatible with FigureCanvasSVG but was left out
on our side, likely because it wasn't a thing at the time this code was written. The list of supported file format will then naturally expand if more formats are added upstream, and reciprocally, this avoids creating a situation where we list a format as supported even though it requires a more recent version of Matplotlib than is installed.

This refactor passes yt/visualization/tests/test_commons.py

for reproduction:

import yt
ds = yt.load_sample('IsolatedGalaxy')
yt.PhasePlot(ds, ('gas', 'density')).save("/tmp/", suffix="svgz")

fails on the main branch, but succeeds here

@neutrinoceros neutrinoceros added enhancement Making something better api-consistency naming conventions, code deduplication, informative error messages, code smells... refactor improve readability, maintainability, modularity labels Jul 4, 2021
@neutrinoceros neutrinoceros force-pushed the dynamical_image_format_support branch 6 times, most recently from 6b22c98 to 49f7441 Compare July 7, 2021 20:41
yt/visualization/_commons.py Show resolved Hide resolved
@neutrinoceros neutrinoceros changed the title refactor: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format. ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format. Aug 30, 2021
@Xarthisius
Copy link
Member

@yt-fido test this please

@neutrinoceros
Copy link
Member Author

I don't remember why you triggered a new job on Jenkins, but I imagine you'd like to revisit this, @Xarthisius ?

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

LGTM; pending question about idioms, good to merge.

if s in cls.get_supported_filetypes():
return cls
raise RuntimeError(
"Something went terribly wrong. "
Copy link
Member

Choose a reason for hiding this comment

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

terribly wrong :)



def normalize_extension_string(s: str) -> str:
return f".{s.lstrip('.')}"
if sys.version_info < (3, 9):
Copy link
Member

Choose a reason for hiding this comment

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

Is this the idiomatic way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It is so idiomatic that it's the only way pyupgrade knows how to clean https://github.com/asottile/pyupgrade#python2-and-old-python3x-blocks
in fact, see : asottile/pyupgrade#530
(notice me sempai !)

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

@matthewturk
Copy link
Member

I'm good with this post resolution

@neutrinoceros
Copy link
Member Author

Alright I'll resolve the conflict now and come back later to rettriger Jenkins !

@neutrinoceros
Copy link
Member Author

@matthewturk I assume you'd want to use the auto-merge button here :)

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@matthewturk matthewturk merged commit c414e45 into yt-project:main Oct 11, 2021
@neutrinoceros neutrinoceros deleted the dynamical_image_format_support branch October 12, 2021 03:19
@neutrinoceros neutrinoceros mentioned this pull request Oct 12, 2021
42 tasks
@matthewturk
Copy link
Member

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 15, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout yt-4.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 c414e45fdcd7ff15afbbe6430ca830e31c704d99
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3410: ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format.'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3410-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3410 on branch yt-4.0.x (ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format.)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 16, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout yt-4.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 c414e45fdcd7ff15afbbe6430ca830e31c704d99
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3410: ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format.'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3410-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3410 on branch yt-4.0.x (ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format.)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

@neutrinoceros
Copy link
Member Author

Backporting this manually with a risk of introducing a new bug in handling the conflict doesn't seem worth it, I'll remove the "Needs Manual Backport label"

neutrinoceros pushed a commit to neutrinoceros/yt that referenced this pull request Nov 29, 2021
…_format_support

ENH: determine compatible matplotlib backend semi-dynamically. Incidentally add support for .svgz image format.
matthewturk added a commit that referenced this pull request Dec 1, 2021
Backport #3410 + #3687 to yt-4.0.x (image filename validation fixes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... bug enhancement Making something better refactor improve readability, maintainability, modularity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants