Fix bug when adding multidimensional marker to signal with more than 2 navigation dimensions with different size

[magnunor]

This pull request fixes a bug when adding multidimensional markers to a signal with 2+ navigation axes, where the axes does not have the same size. (#2080).

Essentially, either BaseSignal.add_marker has to be changed, or the internal mechanism in MarkerBase has to be changed: https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/hyperspy/drawing/marker.py#L166

I opted for changing BaseSignal.add_marker, since there is a smaller chance of breaking anything else...

Changes

• BaseSignal.add_marker: marker navigation dimension will now have to be the inverse of the signal navigation dimension. So if the signal has nav dims (2, 3), the marker must be initialized with a list with shape (3, 2).
• Add unit test for checking this
• Add docstring to BaseSignal.add_marker for how to add markers to a signal like this

Progress of the PR

• Fix bug
• Add docstring
• Add tests
• Ready for review

Minimal example showing the bug

With the current RELEASE_next_patch, the marker will be added, however navigating to the right part of the navigation plot will give an IndexError.

import numpy as np
import hyperspy.api as hs
s = hs.signals.Signal1D(np.arange(2*3*8).reshape(2, 3, 8))
marker_pos_list = np.array([[1, 2], [3, 4], [5, 6]])
marker0 = hs.markers.vertical_line(marker_pos_list, color='red')
s.add_marker(marker0, plot_signal=False, permanent=True)
s.plot()
s.axes_manager.indices = (2, 0)

Trying to "flip" the navigation dimensions to fix this, the marker raise a ValueError when using s.add_marker:

import numpy as np
import hyperspy.api as hs
s = hs.signals.Signal1D(np.arange(2*3*8).reshape(2, 3, 8))
marker_pos_list = np.array([[1, 3, 5], [2, 4, 6]])
marker0 = hs.markers.vertical_line(marker_pos_list, color='red')
s.add_marker(marker0, plot_signal=False, permanent=True)

Using this pull request, the marker can be added, and plotted without giving the IndexError:

import numpy as np
import hyperspy.api as hs
s = hs.signals.Signal1D(np.arange(2*3*8).reshape(2, 3, 8))
marker_pos_list = np.array([[1, 3, 5], [2, 4, 6]])
marker0 = hs.markers.vertical_line(marker_pos_list, color='red')
s.add_marker(marker0, plot_signal=False, permanent=True)
s.plot()
s.axes_manager.indices = (2, 0)

[magnunor]

Might be of interest to pyxem (@dnjohnstone), if they use markers to show where peaks are located.

[ericpre]

A couple of comments:

• the merge conflict need to be solved,
• the error message of your first example is now outdated and not correct anymore,
• it would have been nice to keep the consistency (same "shape" of the list) with the navigation shape,
• I find that the documentation is a bit confusing with respect of the shape of the marker list.

[magnunor]

Merge conflict has been fixed, error message updated and docstring examples has been improved.

it would have been nice to keep the consistency (same "shape" of the list) with the navigation shape,

Indeed, howeer that would require breaking the API. Which would land it in RELEASE_next_major, although this is a bug.

[francisco-dlp]

LGTM, @ericpre ?

[magnunor]

With regards to making the navigation shape and marker input shape the same, we could merge this pull request. Then have an additional pull request for RELEASE_next_major, which changes the internal behaviour.

[francisco-dlp]

The thing is that HyperSpy uses internally C-order, just like numpy, so, I think that using C-order when passing arrays to HyperSpy objects is fine. A way to improve the interface of the markers would be to let them accept HyperSpy signals in addition to arrays. That would make it explicit that the dimensions are actually consistent with HyperSpy conventions.

[magnunor]

A way to improve the interface of the markers would be to let them accept HyperSpy signals in addition to arrays.

That is a pretty good idea! I'm slightly tempted to extend this pull request to handle signals as well, since it makes explaining the dimensions a lot easier. Maybe send it to RELEASE_next_minor instead.

[francisco-dlp]

I think that this one is fine as it is, but we must wait to see if @ericpre agrees. The new feature should indeed go into a separate branch.