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

Set deterministic to True for vector graphics and warn about change to True in future for PNG #197

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

astrofrog
Copy link
Collaborator

While working on this I started to wonder if this should be a kwarg at all for mpl_image_compare or just a CLI/ini option. Any opinions?

@ConorMacBride
Copy link
Member

Most people will want it enabled/disabled for all or none, so the CLI/ini option makes sense. I think also having the kwarg would be useful to some people. You also have #181 which is similar.

@ConorMacBride ConorMacBride mentioned this pull request Apr 3, 2023
22 tasks
@ConorMacBride
Copy link
Member

I see the changes in this PR are already in the documentation (https://pytest-mpl.readthedocs.io/en/latest/configuration.html#whether-to-make-metadata-deterministic) so we should merge this before releasing v0.17.0. This PR looks ready to me? Should we add the CLI and ini options now, or maybe later?

@ConorMacBride ConorMacBride added this to the 0.17.0 milestone Nov 21, 2023
@astrofrog
Copy link
Collaborator Author

@ConorMacBride - I don't have time to work on the CLI and ini options now but if you do, feel free to push to this PR. Otherwise we could just merge without as you said.

@ConorMacBride ConorMacBride marked this pull request as ready for review January 2, 2024 16:19
@ConorMacBride
Copy link
Member

@astrofrog I've added the CLI and INI options here: https://pytest-mpl--197.org.readthedocs.build/en/197/configuration.html#whether-to-make-metadata-deterministic

If this looks okay, feel free to merge. I'll then go ahead and release v0.17.0.

@ConorMacBride
Copy link
Member

Hi @astrofrog, can you please take a quick look through this PR. I'd like to get it merged and publish v0.17.0. The latest pluggy release emits a new warning that makes the bug we fixed in #219 much more apparent -- and it's breaking astropy's CI astropy/astropy#15944 Thanks! 😄

Copy link
Contributor

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

lgtm!

@ConorMacBride ConorMacBride merged commit 3bd3703 into matplotlib:main Feb 13, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants