Bug iterate signal with markers

[AEljarrat]

Description

A signal with markers cannot be iteratively plotted, an error is raised (see MEW below). The origin is that signal.get_current_signal does not copy correctly the markers. This PR tries to fix this by:

• Copying the markers as in signal.__deepcopy__.
• Attending to the marker.auto_update property, data from multidimensional markers is indexed appropriately.
• Iterating a signal produces signals that once plotted show the same markers than in the hyperspectral explorer version.

Of course, this is only a proposal to fix the bug open to discussion, I just try to be constructive ;)

BTW I pushed this branch to the main repository by mistake since my remote was not properly configured. I since deleted this, and I am sorry for the mistake!

Progress of the PR

• Changes implemented
• add tests,
• ready for review.

Minimal example of the bug fix

In this MEW, 4 different point, text and rectangle markers are added to a signal with 3 images (4 markers to each image). These have to replicate once the signal is iterated into 3 new images and plotted.

>>> from skimage.feature import peak_local_max
>>> import scipy.misc
>>> ims = hs.signals.BaseSignal(scipy.misc.face()).as_signal2D([1,2])
>>> index = np.array([peak_local_max(im.data, min_distance=100,
>>>                          num_peaks=4) for im in ims])
>>> # Add multiple markers
>>> for i in range(4):
>>>     xs = index[:, i, 1]
>>>     ys = index[:, i, 0]
>>>     m = hs.plot.markers.point(x=xs, y=ys, color='red')
>>>     ims.add_marker(m, True, False, True)
>>>     m = hs.plot.markers.text(x=10+xs, y=10+ys, text=str(i), color='k')
>>>     ims.add_marker(m, True, False, True)
>>> xs = index[:, :, 1]
>>> ys = index[:, :, 0]
>>> m = hs.plot.markers.rectangle(np.min(xs, 1), np.min(ys, 1), np.max(xs, 1), np.max(ys, 1), 
>>>                                              color='green')
>>> ims.add_marker(m, True, False, True)
>>> # Now try to plot, the first two work out of the box
>>> ims.plot()
>>> ims.deepcopy().plot()
>>> # The PR fixes error in this one:
>>> for im in ims:
>>>     im.plot()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-f7a40dbb4bb0> in <module>()
      1 for imsi in ims:
----> 2     imsi.plot()

~/hyperspy/hyperspy/_signals/signal2d.py in plot(self, colorbar, scalebar, scalebar_color, axes_ticks, saturated_pixels, vmin, vmax, no_nans, centre_colormap, **kwargs)
    268             no_nans=no_nans,
    269             centre_colormap=centre_colormap,
--> 270             **kwargs
    271         )
    272     plot.__doc__ %= BASE_PLOT_DOCSTRING, PLOT2D_DOCSTRING, KWARGS_DOCSTRING

~/hyperspy/hyperspy/signal.py in plot(self, navigator, axes_manager, plot_markers, **kwargs)
   2072         if plot_markers:
   2073             if self.metadata.has_item('Markers'):
-> 2074                 self._plot_permanent_markers()
   2075 
   2076     plot.__doc__ %= BASE_PLOT_DOCSTRING, KWARGS_DOCSTRING

~/hyperspy/hyperspy/signal.py in _plot_permanent_markers(self)
   4578         for marker_name in marker_name_list:
   4579             marker = markers_dict[marker_name]['_dtb_value_']
-> 4580             if marker.plot_marker:
   4581                 if marker._plot_on_signal:
   4582                     self._plot.signal_plot.add_marker(marker)

~/hyperspy/hyperspy/misc/utils.py in __getattribute__(self, name)
    335             name = name.decode()
    336         name = slugify(name, valid_variable_name=True)
--> 337         item = super(DictionaryTreeBrowser, self).__getattribute__(name)
    338         if isinstance(item, dict) and '_dtb_value_' in item and "key" in item:
    339             return item['_dtb_value_']

AttributeError: 'DictionaryTreeBrowser' object has no attribute 'plot_marker'

[francisco-dlp]

This would be a better fit for Release next patch, could you rebase it?

[AEljarrat]

Hi @francisco-dlp, Yes, I can do this, no problem :) Just a question, to be sure; I see a message that you changed the base branch from RELEASE_next_minor to RELEASE_next_patch and back again. I understand the correct branch onto which this one should be rebased is RELEASE_next_patch, is this correct?

[francisco-dlp]

Yes, that's correct, sorry for the misleading message: it was just a quick way to verify if you have branched from Release next patch but sent it to Rnm by mistake.

[AEljarrat]

Done! Instead of rebasing the existing branch, I have branched anew from RELEASE_next_patch and replayed my commits there. Then I have force-pushed this new branch into my remote.

Thankfully I only had a few commits: I ran into many issues when trying to rebase this branch from RNM to RNP. After initially fetching RNP, I tried

git checkout bug_iterate_signal_with_markers
git rebase RELEASE_next_patch

This produced many merging issues, that I was asked to solve separately, from commits spanning several years ago! I was sure something was not going as it should, but I couldn't fix this so I gave up on the rebase approach. Any idea about what I did wrong?

I will try to finish the work on this branch soon, as I had forgotten I still wanted to add some tests.

[AEljarrat]

@francisco-dlp I've finally had some time to implement a test for this PR. I believe that it should be ready now for review.

[francisco-dlp]

LGTM except for the small questions.

[francisco-dlp]

Why do you need this?

[AEljarrat]

I guess you refer to the import BaseSignal? I use a basesignal to test in test_iterate_markers

[francisco-dlp]

No, I meand @update_close_figure

[AEljarrat]

I think that is needed to close the figure that is created when add_marker is called.

[AEljarrat]

On second thought, it seems it is not needed.

[francisco-dlp]

I think that the purpose of this in other tests is to test if closing the plot closes the markers without issue. Having it here may make this test fail if that gets broken what may be confusing. However, @ericpre should know better.

[francisco-dlp]

and this?

[AEljarrat]

This test compares the markers from the sub-signal ims with the markers from the original signal im. It should check if the markers are replicated correctly when the signal is iterated into sub-signals; for im in ims.

[francisco-dlp]

I agree but, why does it have to return ims?

[AEljarrat]

Ups! certainly not. I will remove that together with the update_close_figure.

Thanks for reviewing!

[ericpre]

If I remember correctly, the update_close_figure decorator is to check that the plots are closing without raising any error. It doesn't seem to be required here because this should be covered elsewhere.

[francisco-dlp]

Could you merge Rnm in?

[AEljarrat]

Done! Thanks for the clarifications regarding the update_close_figure decorator.