FIX: add support for imshow extent to have units #22230
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
f10736c to
ceadcea
Compare
ceadcea to
cb3d385
Compare
|
Looks like you forgot to commit an svg? |
Yes I realized that, but pytest generated only pdf and png for me. Is there any explicit command using which I can get the svg? |
|
Yeah you need Inkscape. I think it is also possible to get the test artifacts and use that svg. However I always forget how to do that 😀 |
lib/matplotlib/tests/test_axes.py
Outdated
| @@ -7387,3 +7387,12 @@ def test_clim(): | |||
| clim = (7, 8) | |||
| norm = plot_method(clim=clim).norm | |||
| assert (norm.vmin, norm.vmax) == clim | |||
|
|
|||
|
|
|||
| @image_comparison(["extent_units"]) | |||
There was a problem hiding this comment.
Can you mark this to have the mpl2 style (so we do not test jet + interpolation)?
There was a problem hiding this comment.
and I suspect we can get away with only the pdf or svg tests here (they are not affected by the freetype version issues, we do not not really want to test font rendering here, and any issues we are going to have with units are going to happen well above the renderer level so generating the pdf/png/svg does not actually get us more meaningful test coverage.
There was a problem hiding this comment.
Will make the changes, thank you
|
👍 This was simpler to implement than I expected :) |
|
So this seems simple and makes sense, but I wonder if it's at the right level. If you now call |
2e763a3 to
615c7f6
Compare
615c7f6 to
592bc4d
Compare
592bc4d to
bde0f72
Compare
bde0f72 to
d1b5289
Compare
d1b5289 to
a9654da
Compare
|
@pratimugale are you still working on this? Anything I can do to help out? |
lib/matplotlib/axes/_axes.py
Outdated
| @@ -5374,7 +5374,7 @@ def imshow(self, X, cmap=None, norm=None, aspect=None, | |||
| See the :doc:`/tutorials/intermediate/imshow_extent` tutorial for | |||
| examples and a more detailed description. | |||
|
|
|||
| extent : floats (left, right, bottom, top), optional | |||
| extent : floats or units (left, right, bottom, top), optional | |||
There was a problem hiding this comment.
I would leave this as f!oats:
| extent : floats or units (left, right, bottom, top), optional | |
| extent : floats (left, right, bottom, top), optional |
But add a note in the body that "these values may be unitful and match the units of the Axes'
| (xmin, xmax), (ymin, ymax) = self.axes._process_unit_info( | ||
| [("x", [extent[0], extent[1]]), | ||
| ("y", [extent[2], extent[3]])], | ||
| kwargs) |
There was a problem hiding this comment.
| kwargs) | |
| kwargs) | |
| if len(kwargs): | |
| raise ValueError(f"set_extent did not consume all of the kwargs passed {list(kwargs)!r} were unused") |
I _process_unit_info is very forgiving and is meant to be used in contexts where something else is going to make sure we do not have any extra kwargs. However in this case we are silently drop them on the floor (which in not great because now a typo will get silently ignored and not do what the wants....bugs where I am sure that xnuints is right but not doing what I want are among the most maddening!)
Because the keywords used depends on the type of axes we are sitting on (😱 ) I think that this is the best option?
There was a problem hiding this comment.
made the changes
There was a problem hiding this comment.
The checks have passed, could you please have a look?
There was a problem hiding this comment.
I don't see any check for leftover kwargs?
There was a problem hiding this comment.
I had added a check for the leftover kwargs. Could you please let me know what is supposed to be done when there are kwargs remaining? For eg, if we raise a ValueError as before, then other test cases break - like when cmap is used as a kwarg
There was a problem hiding this comment.
You will need to be more explicit about what gets passed to set_extent from __init__, so that you can raise in the manner noted above.
There was a problem hiding this comment.
done. Does imshow even need to pass kwargs to set_extents? I have removed it. So that the user can pass those keyword arguments by calling set_extents and not in imshow. Does that sound correct?
462ad76 to
2f9ef2a
Compare
|
These changes are breaking other things and will need some rethinking. I'm working on that |
2f9ef2a to
e8c5e4a
Compare
|
@tacaswell @jklymak @QuLogic Could you please review? |
| (xmin, xmax), (ymin, ymax) = self.axes._process_unit_info( | ||
| [("x", [extent[0], extent[1]]), | ||
| ("y", [extent[2], extent[3]])], | ||
| kwargs) |
There was a problem hiding this comment.
I don't see any check for leftover kwargs?
b0e758d to
953ee41
Compare
953ee41 to
8c25858
Compare
8c25858 to
6230b2d
Compare
lib/matplotlib/tests/test_axes.py
Outdated
| @@ -8272,3 +8272,15 @@ def test_bar_all_nan(fig_test, fig_ref): | |||
|
|
|||
| ax_ref.bar([1], [1]).remove() | |||
| ax_ref.bar([1], [1]) | |||
|
|
|||
|
|
|||
| @image_comparison(["extent_units.pdf"], style="mpl20") | |||
There was a problem hiding this comment.
I think we probably need somewhat more extensive testing than this - at least test that the x-axes works as expected? Otherwise looks good.
There was a problem hiding this comment.
Hi, could you please let me know what as expected would mean here?
There was a problem hiding this comment.
You are only checking that dates work for the yaxis, not the xaxis
There was a problem hiding this comment.
ah okay, will add the checks
lib/matplotlib/tests/test_axes.py
Outdated
| dateFirst = np.datetime64('2020-01-01', 'D') | ||
| dateLast = np.datetime64('2020-01-11', 'D') |
There was a problem hiding this comment.
Also should be using snake_case, not camelCase.
| dateFirst = np.datetime64('2020-01-01', 'D') | |
| dateLast = np.datetime64('2020-01-11', 'D') | |
| date_first = np.datetime64('2020-01-01', 'D') | |
| date_last = np.datetime64('2020-01-11', 'D') |
d9f4438 to
db70db2
Compare
|
The changes have been made and the build is green. Requesting a review. |
lib/matplotlib/tests/test_axes.py
Outdated
| @@ -8272,3 +8272,27 @@ def test_bar_all_nan(fig_test, fig_ref): | |||
|
|
|||
| ax_ref.bar([1], [1]).remove() | |||
| ax_ref.bar([1], [1]) | |||
|
|
|||
|
|
|||
| @image_comparison(["extent_units_y_axis.pdf"], style="mpl20") | |||
There was a problem hiding this comment.
I would change these to png - many tests are not run if the pdf converter is not found on an architecture. While you are changing it, I would also just merge these two images on one figure as separate subplots.
There was a problem hiding this comment.
subplots sound good :)
have made the changes.
db70db2 to
c9e9b16
Compare
lib/matplotlib/tests/test_axes.py
Outdated
|
|
||
| @image_comparison(["extent_units.png"], style="mpl20") | ||
| def test_extent_units(): | ||
| mpl.style.use("mpl20") |
There was a problem hiding this comment.
| mpl.style.use("mpl20") |
c9e9b16 to
90a4899
Compare
PR Summary
This PR allows the imshow extent parameter to be expressed in units.
It is with reference to the Bug: #22105
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).