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.

Sign up for GitHub

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

Jump to bottom

enh: store basic software info in png metadata #4553

Merged
merged 3 commits into from
Jul 3, 2023
Merged

enh: store basic software info in png metadata #4553

merged 3 commits into from
Jul 3, 2023

Conversation

Xarthisius
Copy link
Member

@Xarthisius Xarthisius commented Jun 29, 2023 •
edited
Loading

PR Summary

Store information about used software (yt, matplotlib, numpy, PIL) in metadata of generated plots.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

How to test?

  1. Generate simple SlicePlot, ProjectionPlot, off-axis projection / VR png.
  2. Use e.g. ImageMagick to inspect resuting pngs:
$ identify -verbose galaxy0030_offaxis_projection.png | grep Software
    Software: PIL-9.5.0|yt-4.3.dev0
$ identify -verbose *.png | grep Software
    Software: Matplotlib version3.7.1, https://matplotlib.org|NumPy-1.24.2|yt-4.3.dev0

@Xarthisius Xarthisius added the enhancement Making something better label Jun 29, 2023
@Xarthisius
Copy link
Member Author

pre-commit.ci autofix

@neutrinoceros
Copy link
Member

love the idea !

matthewturk
matthewturk previously approved these changes Jun 29, 2023
View reviewed changes
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.

strong approve

@Xarthisius
Copy link
Member Author

pre-commit.ci autofix

@Xarthisius Xarthisius force-pushed the set_software_meta branch 3 times, most recently from 4bfc2af to 2e14b20 Compare June 29, 2023 19:54
neutrinoceros
neutrinoceros reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Maybe InteractiveCamera.save could use some of this too, but I couldn't figure out how, and I'm not even sure it's actually feasible so it's not crucial. Other than that, I only have a couple minor comments

yt/funcs.py Outdated

import numpy as np
from more_itertools import always_iterable, collapse, first

from yt._maintenance.deprecation import issue_deprecation_warning
from yt._version import __version__ as yt_version
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we nest this import in get_version_stack to avoid having to support people relying on from yt.funcs import yt_version in the future. (I know people already use the indirection from yt.funcs import mylog)

yt/funcs.py Outdated
@@ -1447,3 +1448,25 @@ def validate_moment(moment, weight_field):
"Weighted projections can only be made of averages "
"(moment = 1) or standard deviations (moment = 2)!"
)


def setdefault_mpl_metadata(mpl_kwargs: Dict[str, Any], name: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

took me a minute to realise but in this context, I think save_kwargs may be a better name than mpl_kwargs (which is also used in other places for completely different stuff)

Copy link
Member Author

@Xarthisius Xarthisius Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's specifically a matplotlib kwarg {"metadata": {...}}, not just yt's save.

yt/funcs.py Outdated Show resolved Hide resolved
yt/utilities/png_writer.py Outdated


def call_png_write_png(buffer, fileobj, dpi):
Image.fromarray(buffer).save(fileobj, dpi=(dpi, dpi), format="png")
metadata = PngInfo()
metadata.add_text("Software", f"{PIL.__name__}-{PIL.__version__}|yt-{yt_version}")
Copy link
Member

Choose a reason for hiding this comment

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

isn't PIL.__name__ always going to be 'PIL' ? (seems to be the case even with pillow)

@chrishavlin
Copy link
Contributor

Cool! This is great! Don't have anything to add to the review... but I was playing around locally and devised a little self contained python-only example, might be useful for adding a test:

from yt.testing import fake_amr_ds
from yt import SlicePlot
import PIL

ds = fake_amr_ds()
fn = SlicePlot(ds, "x", "Density").save()[0]

with PIL.Image.open(fn) as image:
    assert 'Software' in image.text
    assert 'Matplotlib version' in image.text['Software']

@Xarthisius @neutrinoceros
enh: update after review comments
176e7f3 
Co-authored-by: Clément Robert <cr52@protonmail.com>
@Xarthisius Xarthisius force-pushed the set_software_meta branch 2 times, most recently from dc93451 to 3415d12 Compare June 30, 2023 17:31
neutrinoceros
neutrinoceros reviewed Jun 30, 2023
View reviewed changes
yt/visualization/tests/test_save.py Outdated Show resolved Hide resolved
yt/visualization/tests/test_save.py Outdated Show resolved Hide resolved
yt/visualization/tests/test_save.py Outdated Show resolved Hide resolved
chrishavlin
chrishavlin approved these changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

Actually looked through the code this time -- nothing to add, looks good!

@matthewturk you had a prior review on this, good to go?

@matthewturk matthewturk merged commit 48d6fac into yt-project:main Jul 3, 2023
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrishavlin chrishavlin chrishavlin approved these changes

@neutrinoceros neutrinoceros neutrinoceros approved these changes

@matthewturk matthewturk matthewturk left review comments

Assignees
No one assigned
Labels
enhancement Making something better viz: 2D
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Xarthisius @neutrinoceros @chrishavlin @matthewturk