Intensity log scale plot #1727

ericpre · 2017-09-10T12:32:31Z

Following #1725, this PR add support for plotting signal on a log scale with the intensity_scale argument of the plot, which can be set to 'linear', 'log' or None.:

add intensity_scale to the plot method of the BaseSignal class,
scale can be toggled between linear and logarithmic scale by pressing the l key,
add power_spectrum and shifted parameters to the plot method of complex signals,
plot power spectum with zero frequency component shifted to the center for FFT by default,
add tests,
update doc,
ready for review.

By default if the signal to plot has the Signal.FFT node in the metadata, the power spectrum will be plotted on a log scale (the complex FFT can still be plotted by setting power_spectrum=False).

… the data. Add tests.

ericpre · 2017-10-15T17:06:11Z

Fix #543.

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py

ericpre · 2018-02-19T00:19:17Z

@woozey, in case you fancy having a look! ;)

thomasaarholt · 2018-02-26T10:24:20Z

I've been playing around with the log plotting. Works nicely!

One minor error: My (EELS) spectrum image has some negative values in it, but certainly not all of the values are negative:

>>> eels
<EELSSpectrum, title: EELS Spectrum Image, dimensions: (24, 56|2048)>
>>> eels.T.plot(intensity_scale='log')
ValueError: Data has no positive values, and therefore can not be log-scaled.

thomasaarholt · 2018-02-26T10:45:07Z

fft.plot(shifted=True/False) seems to have no effect on my fft signal.

ericpre · 2018-02-26T14:49:44Z

Does the ValueError comes from matplotlib? Can you give the traceback, to make sure?
I don't think that there is anything sensible to do from our side.

thomasaarholt · 2018-02-26T23:00:17Z

Based on a quick google search I think you are correct. I can double check tomorrow.

Any thought on the shifted?

… is plotted shifted. For convenience set the same options to the roi plot as the original signal plot.

thomasaarholt · 2018-02-28T07:43:01Z

I haven't got time right now to look for the file with negative counts. I'm pretty sure that this was matplotlib, not you.

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py # hyperspy/drawing/signal1d.py

woozey · 2018-04-16T12:53:29Z

Nice PR! It works fine for me. I'll try some more use cases and will give more feedback. Some comments that I have now:

Plotting shifted FFT by default is good for non-experienced users who just want to see their FFT as they are used to, though for advanced users plotting may give an impression that the FFT itself is shifted. Would it be better warning a user that the display shows shifted FFT while the signal itself is not?
Another one with regards to shifting:

holo.fft(shifted=True).plot()
holo.fft(shifted=False).plot()

Would result in:

whereas following the idea of your implementation it probably has to look the same. Would it be better to check the shifted node in metadata to decide if the display needs to be shifted or not?

And the last one, having in mind that you have done so much work figuring all of that out, it would be nice to have a possibility to plot any other signal on a log scale, something like s.plot(log_scale = True). Would you be able to implement that (probably in different PR) without a lot of extra efforts?

ericpre · 2018-04-16T13:16:14Z

Thanks for the feedback! There are indeed quite a few things which need to be discussed here.

I did look at using the "shifted" node of the metadata, but I ended up not being happy about it.
Regarding the defaults, we could consider having different defaults for different kind of signal, which may become particularly relevant after the split of hyperspy/hyperspy-extension

This PR implements the intensity_scale option to the plot, which should allow you to plot any signal on a log scale. If not, this is a bug! ;)

woozey · 2018-04-16T14:55:27Z

I did look at using the "shifted" node of the metadata, but I ended up not being happy about it.

What is the reason? Is there anything that should be improved there?

Regarding the defaults, we could consider having different defaults for different kind of signal, which may become particularly relevant after the split of hyperspy/hyperspy-extension

I wouldn't say my worry are defaults, as I'm happy to have it shifted by default. My real worry is that it is confusing for a user getting the plots shifted although the actual data is not. So I still would strongly advice to put a warning or something in order to inform a user.

This PR implements the intensity_scale option to the plot, which should allow you to plot any signal on a log scale. If not, this is a bug! ;)

Indeed! I have overseen that for no reason! It works excellent! Thanks!

ericpre · 2018-04-16T16:16:14Z

I don't remember exactly, but I don't think there was any particular reason, I just wanted to keep it simple and also hear what other people would expect as default behaviour before going forward.
Maybe it would be good to change the argument name from shifted to centered in the plot method? So that there is no confusion with the shifted node of the metadata, because this argument is only about having the plot with the (0,0) component in the centre regardless of the actual data.

The problem with warning is that people doesn't like it... maybe we could log this at the "info" level.

Normally, they should be a shortcut "l" (as L) to toggle from linear to log scale.

woozey · 2018-04-17T08:56:18Z

I don't remember exactly...
Maybe it would be good to change the argument name from shifted to centered in the plot method?

This sounds reasonable to me. The name does not really matter but that certainly will help avoiding confusion! Another option could be doing similar to matplotlib.pyplot.imshow(), i.e. calling the parameter origin and passing string to it, but this is up to you.

The problem with warning is that people doesn't like it... maybe we could log this at the "info" level.

Perhaps... I don't know what is the best practice in such cases.

Normally, they should be a shortcut "l" (as L) to toggle from linear to log scale.

This one works too!

ericpre · 2018-07-12T17:46:05Z

Yes, indeed, "origin" got its drawback too... I did as you suggested and did two versions for the name of the parameter:

with "shifted"
with "shift"

I prefer the second one because it is clear what it does regardless if the FFT is shifted or not. Moreover, I find it is more convenient to use with ROI:

import hyperspy.api as hs
holo = hs.datasets.example_signals.object_hologram()
fft = holo.fft(True)
fft.plot(True) # True is for the power spectrum

With the version with "shifted":

import hyperspy.api as hs
holo = hs.datasets.example_signals.object_hologram()
fft = holo.fft(True)
fft.plot(True, shifted=True) # True is for the power spectrum, but we also need to specify `shifted=True` if not the FFT is shifted back...

woozey · 2018-07-17T10:48:11Z

Now I see the point for shift and shifted! Yes, I think shift would be better than. I'll test the latest version in a while.

woozey · 2018-07-17T11:33:01Z

Just gave it a try. Looks good to me!

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py

francisco-dlp

In addition to the small comments I wonder if it wouldn't be better to replace the fft shift kwarg with a shift_fft or maybe better (for discovery) fft_shift method. The main advantage is that it'll enable going from unshifted to shifted and back to unshifted. It'll also make be more explicit that the operation doubles the size in memory of the data temporarily.

francisco-dlp · 2018-08-28T10:03:37Z

hyperspy/drawing/image.py

+                                'not supported, falling back to the default '
+                                '(linear).')
+            # if the `norm` kwargs is passed to matplotlib, we use it
+            norm = kwargs.pop('norm', None)


I would be good to add an example to the docstring and the user guide of how to use mpl's norm to e.g. display images with negative numbers using SymLogNorm

francisco-dlp · 2018-08-28T11:01:57Z

doc/user_guide/tools.rst


-By default both methods calculate FFT and IFFT with origin at (0, 0) (not in the centre of FFT). Use `shifted=True` option to
+By default, both methods calculate FFT and IFFT with origin at (0, 0) (not in the centre of FFT). Use ``shifted=True`` option to


I think that it would be good to mention here that ROIs don't work on "unshifted" FFTs shifted to plot.

francisco-dlp · 2018-08-28T11:06:45Z

hyperspy/docstrings/plot.py

@@ -53,6 +53,10 @@
        plot_markers : bool, default True
            Plot markers added using s.add_marker(marker, permanent=True).
            Note, a large number of markers might lead to very slow plotting.
+        intensity_scale : {None, 'linear', 'log'}, default is None.


What about using "auto" instead of None. The advantage is that it's more explicit. I know that in HyperSpy None often means "auto", maybe something else to fix?

francisco-dlp · 2018-08-28T11:37:15Z

hyperspy/drawing/image.py

+        # set the image normalisation
+        if self.intensity_scale == 'log':
+            from matplotlib.colors import LogNorm
+            norm = LogNorm(vmin=self.vmin, vmax=self.vmax)


What happens if norm is in kwargs and intensity_scale == "log"?

Wouldn't it be clearer if we use norm instead of intensity_scale as the name for this kwarg. If it is a mpl norm, we use it directly, if "linear" or "log" or "auto" we do as it currently does for intensity_scale.

francisco-dlp · 2018-08-28T11:39:11Z

hyperspy/drawing/image.py

+                                'not supported, falling back to the default '
+                                '(linear).')
+            # if the `norm` kwargs is passed to matplotlib, we use it
+            norm = kwargs.pop('norm', None)


If the norm is an mpl norm it could be an instance or a class (currently it assumes that it is an instance). If it is a class, it could create the instance passing vmin and max to the constructor.

francisco-dlp · 2018-08-28T11:52:31Z

hyperspy/_signals/complex_signal2d.py

@@ -57,6 +57,9 @@ def add_phase_ramp(self, ramp_x, ramp_y, offset=0):
        self.phase = phase

    def plot(self,
+             power_spectrum=False,
+             intensity_scale=None,
+             shift=False,


What about calling this shift_fft instead?

francisco-dlp · 2018-08-28T12:02:21Z

hyperspy/signal.py

-            self._plot.signal_title = self.metadata.General.title
+            title = self.metadata.General.title
+            if kwargs.get('power_spectrum', False):
+                title = title.replace('FFT', 'Power spectrum')


This assumes that FFT is contained in the title. It may not be the case because the user has changed it or the data comes from an external source.

francisco-dlp · 2018-08-28T12:03:13Z

hyperspy/signal.py

+            title = self.metadata.General.title
+            if kwargs.get('power_spectrum', False):
+                title = title.replace('FFT', 'Power spectrum')
+                self._plot.quantity_label = 'Power spectral density'


What about the units?

francisco-dlp · 2018-08-28T12:04:02Z

hyperspy/signal.py

@@ -3256,7 +3268,7 @@ def fft(self, shifted=False, **kwargs):

        Parameters
        ----------
-        shifted : bool, optional
+        shift : bool, optional
            If True, the origin of FFT will be shifted in the centre (Default: False).


to the centre?

francisco-dlp · 2018-08-28T12:04:58Z

hyperspy/signal.py

-        im_fft.metadata.set_item('Signal.FFT.shifted', shifted)
+        im_fft.metadata.set_item('Signal.FFT.shifted', shift)
+        if hasattr(self.metadata.Signal, 'quantity'):
+            self.metadata.Signal.__delattr__('quantity')


Why removing the quantity? Couldn't we compute the right one with pint? Same for units.

I am not sure if pint can do it properly.

francisco-dlp

In addition to the comments above I think that it would be better to replace the shift kwarg of fft with a fft_shift method. In this way:

It'll be possible to "unshift" a "shifted" fft
It'll be more explicit that the operations requires temporarily doubling the size in memory.

…uto"`.

ericpre · 2018-08-28T18:44:31Z

In addition to the comments above I think that it would be better to replace the shift kwarg of fft with a fft_shift method. In this way:

It'll be possible to "unshift" a "shifted" fft

This is already the possible, isn't

It'll be more explicit that the operations requires temporarily doubling the size in memory.

I don't see what is doubling the memory?

francisco-dlp · 2018-08-29T05:19:59Z

I may be wrong, but I think that fftshift returns a copy of the data. Therefore, it requires 2x the memory compared to an "unshifted" fft.

francisco-dlp · 2018-08-30T11:30:37Z

@ericpre, is this ready for review?

ericpre · 2018-08-30T11:37:33Z

I may be wrong, but I think that fftshift returns a copy of the data. Therefore, it requires 2x the memory compared to an "unshifted" fft.

Even if it's return a copy (I haven't checked), for plotting it is computed only using the current index, so this is possibly not a big deal in term of memory issue?

Other than that, yes, it is ready for review.

francisco-dlp · 2018-08-30T11:40:10Z

Sorry my point wasn't fully clear: I meant the shifted kwarg in fft. If you prefer, I merge this and I can submit a new PR to turn shfit into a fft_shift method.

ericpre · 2018-08-30T12:48:14Z

Yes, let's do this in another PR.

ericpre added 5 commits September 4, 2017 11:36

Add log_scale option to plot of signal2D with test.

b5a9b17

Add log_scale option to plot of signal1D with test.

6197c8a

Plot of complex data: add options to display power spectrum and shift…

7e67fb5

… the data. Add tests.

Fix issue with passing arguments to plot.

7e9edfc

Rename log_scale to intensity_scale and add docstrings.

474d7e0

francisco-dlp added release: next minor status: needs review type: new feature labels Sep 12, 2017

Add toggle log/linear scale on plot.

a2360f6

francisco-dlp mentioned this pull request Jan 23, 2018

NEW: fft() and ifft() #1725

Merged

ericpre added 3 commits February 10, 2018 17:04

Merge remote-tracking branch 'upstream/RELEASE_next_minor' into inten…

fbf0df3

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py

Plot FFT and add tests.

c11e91e

Update doc and add FFT node to the metadata specifications.

784b26c

ericpre added 2 commits February 26, 2018 22:28

Fix issue with passing plotting argument of signal1d.

818e38d

Fix issue with the axis when plotting shifted FFT.

a0a2d27

ericpre added 2 commits February 26, 2018 23:25

Tweak quantity label when plotting power spectrum.

ab5e597

Add _plot_kwargs attribute to complex signal to check if the signal…

3f44a4c

… is plotted shifted. For convenience set the same options to the roi plot as the original signal plot.

Merge remote-tracking branch 'upstream/RELEASE_next_minor' into inten…

8bd4aa8

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py # hyperspy/drawing/signal1d.py

Rename "shifted" to "shift" and change accordingly the plotting code.

8f9e575

ericpre added status: needs review and removed status: waiting for author labels Jul 20, 2018

Merge remote-tracking branch 'upstream/RELEASE_next_minor' into inten…

ae55bad

…sity_log_scale_plot # Conflicts: # hyperspy/drawing/image.py

ericpre force-pushed the intensity_log_scale_plot branch from 0ec0de2 to ae55bad Compare August 24, 2018 22:23

francisco-dlp requested changes Aug 28, 2018

View reviewed changes

francisco-dlp added status: waiting for author and removed status: needs review labels Aug 28, 2018

francisco-dlp requested changes Aug 28, 2018

View reviewed changes

ericpre added 3 commits August 28, 2018 16:15

Rename shift to fft_shift for the plot methods.

f314aea

Replace intensity_scale and norm parameter and set default to `"a…

afcab1f

…uto"`.

Clarify setting quantity label for signal2D.

4b968df

ericpre added status: needs review and removed status: waiting for author labels Aug 30, 2018

francisco-dlp merged commit 86515e3 into hyperspy:RELEASE_next_minor Aug 30, 2018

francisco-dlp removed the status: needs review label Aug 30, 2018

ericpre mentioned this pull request Aug 30, 2018

Speed up fast background removal #2032

Merged

6 tasks

ericpre deleted the intensity_log_scale_plot branch August 30, 2018 12:54

ericpre removed release: next minor labels Oct 30, 2018

ericpre mentioned this pull request Nov 5, 2019

Fix histogram complex #2272

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intensity log scale plot #1727

Intensity log scale plot #1727

ericpre commented Sep 10, 2017 •

edited

ericpre commented Oct 15, 2017

ericpre commented Feb 19, 2018

thomasaarholt commented Feb 26, 2018

thomasaarholt commented Feb 26, 2018 •

edited

ericpre commented Feb 26, 2018

thomasaarholt commented Feb 26, 2018

thomasaarholt commented Feb 28, 2018

woozey commented Apr 16, 2018

ericpre commented Apr 16, 2018

woozey commented Apr 16, 2018

ericpre commented Apr 16, 2018

woozey commented Apr 17, 2018

ericpre commented Jul 12, 2018

woozey commented Jul 17, 2018

woozey commented Jul 17, 2018

francisco-dlp left a comment

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

francisco-dlp Aug 28, 2018

ericpre Aug 28, 2018

francisco-dlp left a comment

ericpre commented Aug 28, 2018

francisco-dlp commented Aug 29, 2018

francisco-dlp commented Aug 30, 2018

ericpre commented Aug 30, 2018

francisco-dlp commented Aug 30, 2018

ericpre commented Aug 30, 2018


		By default both methods calculate FFT and IFFT with origin at (0, 0) (not in the centre of FFT). Use `shifted=True` option to
		By default, both methods calculate FFT and IFFT with origin at (0, 0) (not in the centre of FFT). Use ``shifted=True`` option to

Intensity log scale plot #1727

Intensity log scale plot #1727

Conversation

ericpre commented Sep 10, 2017 • edited

ericpre commented Oct 15, 2017

ericpre commented Feb 19, 2018

thomasaarholt commented Feb 26, 2018

thomasaarholt commented Feb 26, 2018 • edited

ericpre commented Feb 26, 2018

thomasaarholt commented Feb 26, 2018

thomasaarholt commented Feb 28, 2018

woozey commented Apr 16, 2018

ericpre commented Apr 16, 2018

woozey commented Apr 16, 2018

ericpre commented Apr 16, 2018

woozey commented Apr 17, 2018

ericpre commented Jul 12, 2018

woozey commented Jul 17, 2018

woozey commented Jul 17, 2018

francisco-dlp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francisco-dlp left a comment

Choose a reason for hiding this comment

ericpre commented Aug 28, 2018

francisco-dlp commented Aug 29, 2018

francisco-dlp commented Aug 30, 2018

ericpre commented Aug 30, 2018

francisco-dlp commented Aug 30, 2018

ericpre commented Aug 30, 2018

ericpre commented Sep 10, 2017 •

edited

thomasaarholt commented Feb 26, 2018 •

edited