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

Fix image metadata being used in wrong cell #599

Closed

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented May 7, 2024

Fix bug pointed out in #588 (review) caused by reusing the image_options dict. Fixes #605.

Seems like in my previous PR I didn’t understand the tests enough to realize that I changed them to nonsense, but now I do.

Sorry for that!

cc @OriolAbril

Copy link
Contributor Author

@flying-sheep flying-sheep May 7, 2024

Choose a reason for hiding this comment

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

I changed the metadata of the plot to reflect the text/plain representation (<Figure size 432x288 with 1 Axes>). This results in the plot looking blurry since the image data’s resolution is smaller (390×248). While it doesn’t look nice in the test notebook, it makes the most sense like that.

(The perfect solution would be to have quadruple-sized “retina” image data – e.g. 800×600 – with the metadata being half that – 400×300 – which would match real-world usage)

@flying-sheep flying-sheep changed the title Fix image metadata Fix image metadata being used in wrong cell May 7, 2024
Copy link
Contributor

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Changes look good and I didn't notice anything strange after testing the PR locally. Thanks for the quick fix @flying-sheep!

@juanitorduz
Copy link

Hi! It would be great to have this fix into the next release 😄.

@agoose77
Copy link
Collaborator

agoose77 commented Jun 1, 2024

Thanks for your feedback everyone! I am doing a round of maintenance on the Sphinx stack at the moment, I will aim for a release next week.

@flying-sheep
Copy link
Contributor Author

Hi @agoose77 are you still planning to do a release soon?

@agoose77
Copy link
Collaborator

Thanks @flying-sheep, this is very helpful. Another contributor also opened a PR #609, so I'm in the fortunate position of having to make a choice between the two! As there's a new test in #609, I'm going to close this in favour of that one :)

@agoose77 agoose77 closed this Jun 27, 2024
@OriolAbril
Copy link
Contributor

@agoose77 I would merge both PRs actually. They do have the addition of the .copy() in common, but that is all, so I would not expect merge conflicts either. This PR fixes the existing tests ehich are as of now testing that the wrong behaviour happens (consequently, all tests are failing on the other PR). The other PR doesn't change anything about existing tests (which needs to happen too in addition to fixing the bug) and adds a new test that is more specific for image metadata.

If you prefer, I can also merge both PRs into one keeping the commits of each author so contributions are properly tracked and open a new with the changes from both PRs

@agoose77
Copy link
Collaborator

@OriolAbril yep, I'm pulling the changes from the tests to attribute @flying-sheep :)

@flying-sheep flying-sheep deleted the fix-metadata branch June 27, 2024 12:13
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.

Always “config changed ('nb_render_image_options')”
4 participants