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

Add tests for ImageGrid #23553

Merged
merged 1 commit into from Sep 25, 2022
Merged

Add tests for ImageGrid #23553

merged 1 commit into from Sep 25, 2022

Conversation

oscargus
Copy link
Contributor

@oscargus oscargus commented Aug 4, 2022

PR Summary

Add tests for ImageGrid.

Minor correction to docs and code as one can get the impression that a string can be passed as rect, but this will give an error later on.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@QuLogic
Copy link
Member

QuLogic commented Aug 9, 2022

Oh, also, do you need text; can you set remove_text=True?

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I suggest to connect the colorbars with the respective axes images, so that they are not empty. C.f. https://matplotlib.org/stable/gallery/axes_grid1/demo_axes_grid.html

Also, not sure if it’s worth it here, but I generally try to introduce asymmetry into tests, e.g. (2,3) instead of (2,2) so that the test gets sensitive to a mixup of rows/columns.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 9, 2022

Oh, also, do you need text; can you set remove_text=True?

I also test label_mode, so the text is required to see the effect.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 9, 2022

I suggest to connect the colorbars with the respective axes images, so that they are not empty. C.f. https://matplotlib.org/stable/gallery/axes_grid1/demo_axes_grid.html

Also, not sure if it’s worth it here, but I generally try to introduce asymmetry into tests, e.g. (2,3) instead of (2,2) so that the test gets sensitive to a mixup of rows/columns.

Sure, I can modify the tests like that.

@oscargus oscargus marked this pull request as draft August 9, 2022 19:18
@oscargus oscargus marked this pull request as ready for review August 10, 2022 09:55
@oscargus
Copy link
Contributor Author

This is updated based on the comments, including expanding the test names to tell what is tested.

It is maybe not a good idea to test two features, colorbar and label_mode in the same test, but there will be quite a lot of images testing all combinations...

@timhoffm
Copy link
Member

timhoffm commented Aug 10, 2022

This is updated based on the comments, including expanding the test names to tell what is tested.

It is maybe not a good idea to test two features, colorbar and label_mode in the same test, but there will be quite a lot of images testing all combinations...

As far as I understand, label_mode and colorbar do not depend on each other, so explicit combinations of these need not be tested.

label_mode can easily be tested algorithmically by checking visibility attributes. I don't think we need images for that. This has the added benefit that these tests become independent of the visual representation of ticks and labels.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 10, 2022

I was a bit unclear. I was referring to combination of colorbar combinations ('each', 'single', 'edge') x ('left', 'right', 'top', 'bottom').

Not so clear why the colorbar content should be in the test (which is not at all related to ImageGrid as such), but the text should not (which allows visible inspection of what should happen).

@timhoffm timhoffm added this to the v3.7.0 milestone Sep 25, 2022
@timhoffm timhoffm merged commit 0c5b792 into matplotlib:main Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants