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

Use pcolormesh for image plots involving non-uniform axes #3192

Merged
merged 6 commits into from Sep 8, 2023

Conversation

pietsjoh
Copy link
Contributor

@pietsjoh pietsjoh commented Jul 14, 2023

Description of the change

As mentioned in #2398 it would be better to use pcolormesh instead of imshow for image plots with non-uniform axes.
This switch doesn't affect image plots with only uniform axes. It only gets used when atleast 1 of the 2 axes of the image is non-uniform.

As far as I can tell, there are essentially 3 different ways to get an image plot:

  • (A) 2D sig
  • (B) 1D sig, 1D nav
  • (C) 2D nav

As for (A): implementing this is not a problem, I just need to check if the update step works as intended.
Plotting (B) and (C) also involves widgets. The line widgets seem to also work with pcolormesh and thus (B) also works. However, case (C) is more tricky. The SquareWidget updates the position correctly, but not the size of the widget. Switching to RectangularWidget looks difficult, because it heavily relies on the scale attribute of the axis.

I will check how to update the size, but I am not sure if this functionality is necessary. Because most non-uniform axes are signal axes anyways, hence supporting pcolormesh for 2D nav plots might not be widely used.

Progress of the PR

  • implement image plots with pcolormesh
    • 2d sig
    • 1d sig, 1d nav
    • confirm that update step works as intended
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

import hyperspy.api as hs
import numpy as np
import 
matplotlib.pyplot as plt
xa = {"axis": np.linspace(0, 1, 20)**2, "name": "X"}
ya = {"axis": np.linspace(0, 1, 20)**2, "name": "Y"}
np.random.random((20, 20))
s = hs.signals.Signal2D(np.random.random((20, 20)), axes=[ya, xa])
s.plot()
plt.show()

Plot generated by the code above.
top=current(imshow)
bottom=change(pcolormesh)

example_imshow
example_pcolormesh

Notes

I based this on RELEASE_next_minor, but I am unsure if this correct. This PR does not affect hyperspy's api directly. However, pcolormesh accepts different args than imshow. For example s.plot(aspect=100) passes aspect to imshow. But pcolormesh doesn't have this arg and will throw an error. Thus backwards compatibility might be broken for some scripts.

@pietsjoh pietsjoh changed the title Pcolormesh Use pcolormesh for image plots involving non-uniform axes Jul 14, 2023
@jlaehne
Copy link
Contributor

jlaehne commented Jul 14, 2023

I will check how to update the size, but I am not sure if this functionality is necessary. Because most non-uniform axes are signal axes anyways, hence supporting pcolormesh for 2D nav plots might not be widely used.

Indeed case A and B are the most important ones. Case B is quite common, as the navigator of a linescan could be such a plot (currently it is a lineplot of the integrated intensity). So I would put priority on these two cases for the moment.

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.01% ⚠️

Comparison is base (c1a0de8) 81.25% compared to head (6865062) 81.24%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3192      +/-   ##
======================================================
- Coverage               81.25%   81.24%   -0.01%     
======================================================
  Files                     173      173              
  Lines                   24192    24202      +10     
  Branches                 5621     5624       +3     
======================================================
+ Hits                    19657    19663       +6     
- Misses                   3236     3238       +2     
- Partials                 1299     1301       +2     
Files Changed Coverage Δ
hyperspy/drawing/image.py 74.65% <73.68%> (-0.23%) ⬇️
hyperspy/signal.py 77.62% <100.00%> (-0.03%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

I based this on RELEASE_next_minor, but I am unsure if this correct.

In principle that would be still OK, but there will be no more minor release before version 2.0 envisaged for september. So if this PR can make it into v2.0, it will get into a major release.

@jlaehne jlaehne added this to the v2.0 Split milestone Aug 7, 2023
@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

The tests on azure pipelines are failing ... I guess some images are now different than tested for?

@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

I tested the implementation on some real data (linescans, thus the 1d sig, 1d nav case using the image plot as navigator). Looks good.

@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

A point I noticed, also when testing hyperspy/rosettasciio#87, is that for non-uniform axes, the aspect ratio of image plots in not really ideal (x-axis is much shorter than y-axis). It is not possible to correct this using the min_aspect argument, which does not seem to have an influence. But, maybe this is an issue for a separate PR.

Also, for navigators as well as for plot_spectra(s, style='heatmap') it is not possible to change the cmap. But that definitely would be a separate PR.

@pietsjoh pietsjoh changed the base branch from RELEASE_next_minor to RELEASE_next_major August 9, 2023 09:25
@pietsjoh
Copy link
Contributor Author

pietsjoh commented Aug 9, 2023

The tests on azure pipelines are failing ... I guess some images are now different than tested for?

Yes, TestPlotNonLinearAxis.test_plot_non is expected to fail for test_plot_non_uniform_nav, because the default navigator is switched from spectrum to data and thus the navigator plot should differ. Although I find it surprising that this test only fails on azure pipelines and neither on my local machine nor on the other test jobs.

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Aug 9, 2023

A point I noticed, also when testing hyperspy/rosettasciio#87, is that for non-uniform axes, the aspect ratio of image plots in not really ideal (x-axis is much shorter than y-axis). It is not possible to correct this using the min_aspect argument, which does not seem to have an influence. But, maybe this is an issue for a separate PR.

I already experimented with the aspect ratio, also due to hyperspy/rosettasciio#87. Here are my takeaways:

For uniform images, the axis scale is used to set the default aspect ratio. Thus images with axes having the same size always result in square images (independent of scale/offset). If the axes sizes are too different, min_aspect scales the aspect ratio to ensure that the image is not too skewed.

For non-uniform axes, the aspect ratio is always set to 1 independent of min_aspect or the axis sizes. I theoretically have a commit lined up, where I used an average scale to set the aspect ratio for non-uniform axes similar to uniform axes (which seems to work for both imshow and pcolormesh). As this only affects images with non-linear axes anyways I would put it either in here or do a seperate PR afterwards.

Furthermore, there is to add that for imshow one can change the aspect ratio via: s.plot(aspect=100). This doesn't work for pcolormesh, because pcolormesh doesn't have an aspect argument.

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Aug 9, 2023

The tests on azure pipelines are failing ... I guess some images are now different than tested for?

Yes, TestPlotNonLinearAxis.test_plot_non is expected to fail for test_plot_non_uniform_nav, because the default navigator is switched from spectrum to data and thus the navigator plot should differ. Although I find it surprising that this test only fails on azure pipelines and neither on my local machine nor on the other test jobs.

Found the problem, didn't run pytest with --mpl.

@CSSFrancis
Copy link
Member

@pietsjoh it might be also good to add a test which makes sure that the tests are plotting correctly using the --mpl compare.

If you haven't already looked https://hyperspy.org/hyperspy-doc/dev/dev_guide/testing.html#plot-testing this is a good reference for how to add those tests.

Just a note that those tests require specific versions of matplotlib and a couple of other packages to work properly.

The plotting tests are tested on Azure Pipelines against a specific version of matplotlib defined in conda_environment_dev.yml. This is because small changes in the way matplotlib generates the figure between versions can sometimes make the tests fail.

@pietsjoh
Copy link
Contributor Author

I have updated the existing non-uniform axes plot tests. The tests cover both implemented cases (1d sig, 1d nav; 2d sig). However, I would prefer using structured data instead of random data. I think this would be better to detect any problems when the test fails. So should I add a new test or modify the existing ones?

Furthermore, a test is needed to check whether the update works as intended when changing the navigation coordinates. Here I am unsure how to do this exactly. To be more specific: How do I change the navigation coordinates for plot (behaviour of "Ctrl+ Arrow" for example) in a python script?

@pietsjoh
Copy link
Contributor Author

Furthermore, a test is needed to check whether the update works as intended when changing the navigation coordinates. Here I am unsure how to do this exactly. To be more specific: How do I change the navigation coordinates for plot (behaviour of "Ctrl+ Arrow" for example) in a python script?

After a suggestion from @jlaehne I used axes_manager[0].index +=1 to test the update step. If there is a preferred solution let me know.

@jlaehne jlaehne marked this pull request as ready for review August 16, 2023 15:01
@jlaehne
Copy link
Contributor

jlaehne commented Aug 30, 2023

LGTM, anyone else want to have a look @CSSFrancis @ericpre ?

@ericpre
Copy link
Member

ericpre commented Aug 31, 2023

Is there any reason to use matplotlib.axes.Axes.pcolormesh in favour of matplotlib.image.NonUniformImage https://matplotlib.org/stable/gallery/images_contours_and_fields/image_nonuniform.html? The later should be more consistent with the existing implementation? Also would it be possible to use aspect with NonUniformImage?

@jlaehne
Copy link
Contributor

jlaehne commented Aug 31, 2023

Is there any reason to use matplotlib.axes.Axes.pcolormesh in favour of matplotlib.image.NonUniformImage https://matplotlib.org/stable/gallery/images_contours_and_fields/image_nonuniform.html? The later should be more consistent with the existing implementation? Also would it be possible to use aspect with NonUniformImage?

Good question, I have always used pcolormesh for maps with non-uniform axes. As far as I can see, it is the equivalent to imshow, which we use for uniform axes. In particular, NonUniformImage "is not available via an Axes method" (the way we currently call it in the code). So I would say that pcolormesh might actually be more consistent with the current implementation?

It seems to be hard though to find a comparison of the two methods. Though matplotlib/matplotlib#18652 (comment) suggests that NonUniformImage might be deprectated at some point.

Though with pcolormesh, we might have to be aware that "x,y values that represent the corners of the mesh, not the center points": matplotlib/matplotlib#18652 (comment)

@ericpre
Copy link
Member

ericpre commented Aug 31, 2023

NonUniformAxes is not accessible through the matplotlib.axes.Axes API but it is a subclass of matplotlib.image.AxesImage and in that sense is consistent with matplotlib.axes.Axes.imshow (returns matplotlib.image.AxesImage) while matplotlib.axes.Axes.pcolormesh returns a matplotlib.collections.QuadMesh.

If you think that pcolormesh is more suitable, then that's good.
It sounds like this is something that should be sorted in matploltib - keeping one of the two - but this is most likely not trivial.

@pietsjoh
Copy link
Contributor Author

Is there any reason to use matplotlib.axes.Axes.pcolormesh in favour of matplotlib.image.NonUniformImage

I didn't know about matplotlib.image.NonUniformImage. I will check it out tomorrow.

@ericpre
Copy link
Member

ericpre commented Aug 31, 2023

Well, I didn't know neither, but I didn't expect to see that it is necessary to use a collections, so I started to look it up in matplotlib and came across NonUniformImage.

@pietsjoh
Copy link
Contributor Author

pietsjoh commented Sep 8, 2023

So I looked a little deeper into pcolormesh vs NonUniformImage. As @ericpre mentioned, NonUniformImage fits better into the code, because it is more consistent with imshow. This means that all the if mesh: are not needed. The implementation for NonUniformImage would look something like this:

if self.yaxis.is_uniform and self.xaxis.is_uniform:
    ...
else:
    new_args_non_uniform = dict()
    new_args_non_uniform.update(new_args)
    new_args_non_uniform.pop("vmin", None)
    new_args_non_uniform.pop("vmax", None)
    new_args_non_uniform.pop("aspect", None)
    im = matplotlib.image.NonUniformImage(self.ax, **new_args_non_uniform)
    im.set_data(self.xaxis.axis, self.yaxis.axis, data)
    self.ax.add_image(im)
    self.ax.set_xlim(self._extent[0], self._extent[1])
    self.ax.set_ylim(self._extent[2], self._extent[3])

The following image showcases the mentioned difference in representation for a simple plot. pcolormesh automatically uses a full pixel for the boundaries, which is the similar to what hyperspy does for uniform images. For NonUniformImage this can be controlled via set_xlim and set_ylim (extent doesn't seem to work). Thus one could theoretically reproduce the pcolormesh plot. For uniform images hyperspy adds half a scale to extent for each side (-> full pixel at boundaries). This is not easy to generalize for non-uniform images. Thus I would leave it like this if we go with NonUniformImage. Moreover, using more pixel will reduce the visible difference anyways.

pcolormesh_vs_NonUniformImage

The following code reproduces the figure.

import numpy as np
import matplotlib.pyplot as plt
from matplotlib.image import NonUniformImage

x = np.linspace(-4, 4, 3)
y = np.linspace(-4, 4, 3)
z = np.sqrt(x[np.newaxis, :]**2 + y[:, np.newaxis]**2)

fig, axs = plt.subplots(ncols=2, nrows=1)
ax = axs[0]
im = NonUniformImage(ax, extent=(-4, 4, -4, 4))
im.set_data(x, y, z)
ax.add_image(im)
ax.set_xlim(-4, 4)
ax.set_ylim(-4, 4)
ax.set_title("NonUniformImage")

axs[1].pcolormesh(x, y, z)
axs[1].set_title("pcolormesh")

plt.show()

Let's have a look at the extra arguments. In hyperspy these are tailored for imshow currently. pcolormesh doesn't support extent and aspect. However, extent is handled automatically similar to uniform images and therefore not needed anyways. Setting aspect is duplicated anyways, once via self.ax.set_aspect(self._aspect) and also via the arg. The former works in any case. Not just for pcolormesh, but also for NonUniformImage, which doesn't support the aspect arg either. Moreover, NonUniformImage doesn't have vmin and vmax as arguments. I haven't looked into the details of how these are used within hyperspy.

Finally, I want to give a comprehensive list of the pros and cons for using NonUniformImage over pcolormesh. The first 2 points are the most important in my opinion.

  • (+) fits better into the existing code due to the similarity with imshow
  • (-) as mentioned by @jlaehne, this doesn't seem to be widely used and might be deprecated
  • (-) vmin and vmax args cannot be used
  • (-) handling the extent via set_xlim() and set_ylim() similar to uniform case might be difficult

I am happy to implement either one. Just let me know which one you prefer.

@jlaehne
Copy link
Contributor

jlaehne commented Sep 8, 2023

Thanks @pietsjoh for this detailed comparison! In the end, I would still tend to pcolormesh, because the result will look closer to the one with imshow, even though the underlying class is a different one.

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

Thank you @pietsjoh, indeed, it seems that pcolormesh is more sensible, particularly for the vmin/vmax.
A quick search on github (during which I came across matplotlib/matplotlib#7763) also showed that it is clearly more widely used than NonUniformImage.

hyperspy/drawing/image.py Outdated Show resolved Hide resolved
hyperspy/drawing/image.py Outdated Show resolved Hide resolved
hyperspy/drawing/image.py Outdated Show resolved Hide resolved
@ericpre ericpre merged commit 07f8177 into hyperspy:RELEASE_next_major Sep 8, 2023
23 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.

None yet

4 participants