Large restructure of differential phase contrast class structure #1057

magnunor · 2024-04-07T18:11:13Z

This pull request implements the changes discussed in #1039

This to make the pyXem class structure a bit clearer, by changing the default plotting behavior in the BeamShift class:

All the DPCSignal functionality has been moved to the BeamShift class
Some unit tests with random numbers which were prone to failing randomly has been removed
Some rarely used functions which are easy to implement via s.map in one or two lines of code has been removed

This pull request drastically break the API, so this should be implemented in an "API-breaking" release. Also, due to changes in other pull requests, there will probably be some merge conflicts.

Checklist

Some points for discussion:

BeamShift.plot() : currently, the plot function simply does self.T.plot(). Is this a good way of doing this?
Many of the BeamShift functions will only work with signals with 2 navigation dimensions. How to best deal with this?
There is both a get_phase_signal and phase_retrivial, which returns very different phases: get_phase_signal is the "direction" of the beam shift "vector", while phase_retrival retrives the phase.
make_bivariate_histogram (which is not a class method) should be moved to some utils package?
pixelated_stem_tools should be cleaned up, with some of the DPC specific functionality being moved to a (new?) utils focusing on beam-shift and DPC.
I removed gaussian_blur, as (in my experience) it is not used very often, and can easily be replaced by two lines of code.

- All the DPCSignal functionality has been moved to the BeamShift class - BeamShift.plot() default plotting has been changed - Some unit tests with random numbers which were prone to failing randomly has been removed - Some rarely used functions has been removed

coveralls · 2024-04-07T18:30:30Z

coverage: 92.648% (-0.4%) from 93.0%
when pulling b9f881f on magnunor:restructure_dpc_functionality
into 25b6f68 on pyxem:main.

CSSFrancis · 2024-04-08T20:29:44Z

@magnunor I would rebase this based on PR that just got merged before you go too much further as that PR moves some things around.

Let me know if you need any help here. I would like to make this a subclass of Vector1D but I might have to think about how to make that class a little more robust.

magnunor · 2024-04-10T08:12:46Z

@magnunor I would rebase this based on PR that just got merged before you go too much further as that PR moves some things around.

Done! It was actually a lot less work than I feared :)

I would like to make this a subclass of Vector1D but I might have to think about how to make that class a little more robust.

I haven't followed the vector signals development too closely, but I think it makes a lot of sense to utilize a signal optimized for vectors for BeamShift and DPC data.

- Make new _beam_shift_tools module - Remove Diffraction2D.center_of_mass: -- Move to get_direct_beam_position(method="center_of_mass") - Update many unit tests due to these changes - Update many docstrings due to these changes

magnunor · 2024-04-14T18:28:47Z

Some more progress, major remaining thing is the documentation.

The number of added and deleted lines looks daunting, but most of those are due to files having been deleted with the code being moved to a different module. For example the DPCSignal functionality being moved to BeamShift.

I moved BeamShift.get_color_image_with_indicator to utils.plotting, as it returns a matplotlib object, and is mostly used for making image files rather than interactive visualization. Especially with the possibility to include the color wheel in get_color_signal via Better DPC Visualization #1031
I think Diffraction2D.center_direct_beam does pretty much the same thing as Diffraction2D.shift_diffraction, so Diffraction2D.shift_diffraction can probably be removed
I removed Diffraction2D.center_of_mass, as the same functionality is in get_direct_beam_position. However, there needs to be some additional work on the masking functionality.
In get_direct_beam_position(method="center_of_mass") there is both mask and signal_slice which serves a similar purpose. However, for the magnetic DPC processing, it is advantageous that the mask is circular. Not sure what the best solution for this is, but the current way is not optimal.
I made a new utils._beam_shift_utils module
Much more...

I'll make a proper list of the all the changes when they're all implemented.

magnunor · 2024-04-21T18:32:23Z

Two points which would be nice to discuss:

In this pull request, the BeamShift.plot function does this:

def plot(self):
        self.T.plot()

I'm not sure if this is the best ways of doing this? I'm thinking at least the args and kwargs should be passed. But I'm also wondering if this sub-optimal since the self.T creates a new signal which is discarded after the plotting is done.

In get_direct_beam_position(method="center_of_mass") there is both mask and signal_slice which serves a similar purpose. However, for the magnetic DPC processing, it is advantageous that the mask is circular, while signal_slice is a square. Not sure what the best solution for this is, but the current way is not optimal.

@CSSFrancis @pc494

CSSFrancis · 2024-04-21T22:13:21Z

Two points which would be nice to discuss:

In this pull request, the BeamShift.plot function does this:
def plot(self):
        self.T.plot()
I'm not sure if this is the best ways of doing this? I'm thinking at least the args and kwargs should be passed. But I'm also wondering if this sub-optimal since the self.T creates a new signal which is discarded after the plotting is done.

I think we want to do a couple of things:

If the signal is lazy then don't plot it. You pretty much have to compute the resulting shifts if you want to visualize them
Make it easier to plot over a signal if you want to just quickly check to make sure it did decently well.
Plot the shifts. I like to use the hs.plot.plot_images for this. Would a plot_shifts function be useful which just uses hs.plot.plot_images to plot the transpose.

In get_direct_beam_position(method="center_of_mass") there is both mask and signal_slice which serves a similar purpose. However, for the magnetic DPC processing, it is advantageous that the mask is circular, while signal_slice is a square. Not sure what the best solution for this is, but the current way is not optimal.

There is also the half_square_width to make things worse... I added the signal_slice because sometimes my beam center was way off and half_square_width didn't cut it.

The mask implementation should be better as well... But I'm not sure what the best way to handle that is....

@CSSFrancis @pc494

magnunor · 2024-05-03T18:58:54Z

I switched BeamShift.plot over to using hs.plot.plot_images, which was a really good idea @CSSFrancis :)

CSSFrancis

@magnunor This is starting to look quite good. Just some small changes that I might make if you are okay with that?

@pc494 What are your feelings on breaking the API here? There are quite a few changes to the API here. I know we have been trying to avoid that but we might be reaching the end game where we need to start :).

CSSFrancis · 2024-04-07T18:22:30Z

pyxem/signals/beam_shift.py

+        s_bs = self._deepcopy_with_new_data(plane_image)
+        return s_bs
+
+    def plot(self):


Add **kwargs here for passing to plot. Also add documentation saying this is equivalent to .T.plot

CSSFrancis · 2024-05-28T02:13:46Z

pyxem/signals/beam_shift.py



 class BeamShift(Signal1D):
    """Signal class for working with shift of the direct beam."""

    _signal_type = "beam_shift"

-    def make_linear_plane(self, mask=None):
+    def get_linear_plane(self, mask=None, fit_corners=None):


It might be worth testing this with a 5D dataset to make sure that it works with N-Dimensions.

Currently this doesn't work on 5 dimensions but we can wait on that for a later PR.

If we can assume that the probe positions are always the two first navigation dimensions, we could easily iterate over the remaining navigation dimensions.

We also need to handle BeamShift signals with 1 navigation dimension.

CSSFrancis · 2024-05-28T02:23:17Z

pyxem/signals/beam_shift.py

 from hyperspy._signals.lazy import LazySignal
-from hyperspy._signals.signal1d import Signal1D
+from hyperspy.signals import Signal1D
+from hyperspy.utils.plot import plot_images


 class BeamShift(Signal1D):


@magnunor is it okay if I make a couple of adjustments to this? I think we should make BeamShift a subclass of Vector1D. I'm planning on moving some of the plotting capablilites to that class and then that can be a more general feature. I think the other place where that might be useful is with the StrainMap class which is essentially plotting 4 Vector quantities.

CSSFrancis · 2024-05-29T13:27:44Z

@magnunor Do you know why those tests are failing? This seems like a regression but maybe it wasn't tested before.

CSSFrancis · 2024-05-29T13:29:01Z

Once #1080 is merged do you want to rebase? If we figure out the testing I can take another pass through and then we can think about what it would take to merge?

magnunor · 2024-05-29T19:02:24Z

Once #1080 is merged do you want to rebase? If we figure out the testing I can take another pass through and then we can think about what it would take to merge?

Sounds good! I'll do that either late tomorrow, or (most likely) Friday.

CSSFrancis · 2024-05-29T19:17:21Z

Once #1080 is merged do you want to rebase? If we figure out the testing I can take another pass through and then we can think about what it would take to merge?

Sounds good! I'll do that either late tomorrow, or (most likely) Friday.

Perfect!

magnunor · 2024-05-31T14:49:03Z

I merged the current main branch into this branch.

Running BeamShift.plot() on a dataset with <undefined> units gives:

WARNING | Hyperspy | Axes labels were requested, but one or both of the axes units and/or name are undefined. Axes decorations have been set to 'ticks' instead. (hyperspy.drawing.utils:1167)

This will be a very annoying warning, so we should do something to avoid that.

Do you know why those tests are failing? This seems like a regression but maybe it wasn't tested before.

With regards to the failing tests: this is related to interaction between method="center_of_mass" and sig_slice in get_direct_beam_position. So I'm tempted to leave it for the rework of get_direct_beam_positions which I think we should do before the release: #1079 (comment)

CSSFrancis · 2024-05-31T16:23:58Z

I merged the current main branch into this branch.

Yay

Running BeamShift.plot() on a dataset with <undefined> units gives:
WARNING | Hyperspy | Axes labels were requested, but one or both of the axes units and/or name are undefined. Axes decorations have been set to 'ticks' instead. (hyperspy.drawing.utils:1167)
This will be a very annoying warning, so we should do something to avoid that.

In this case we know the units are pixels, right? We can just set that? I think we should pass the axes_manager of the original signal as well to the beam shift and maybe allow pixel --> real units?

Do you know why those tests are failing? This seems like a regression but maybe it wasn't tested before.

With regards to the failing tests: this is related to interaction between method="center_of_mass" and sig_slice in get_direct_beam_position. So I'm tempted to leave it for the rework of get_direct_beam_positions which I think we should do before the release: #1079 (comment)

Hmmm I don't love merging non-passing code into main, but it might be nice to seperate that into a smaller PR.

CSSFrancis · 2024-06-01T13:11:36Z

@magnunor do you have time to work on this or do you want me to give it a shot. On second thought I think the warnining is totally reasonable... People should have axes labels on their data including units. The more we can pester people into doing that (I think) the better, if it's really that annoying it's only a warning, you can silence it.

I can look into fixing that failing test/adjusting it and then we can think about what merging might look like. It might also be nice to make a small Example using a dummy test dataset to show how to do some of the more simple operations.

magnunor · 2024-06-03T07:09:06Z

@magnunor do you have time to work on this or do you want me to give it a shot.

Go for it! My schedule got a bit too full the next days.

On second thought I think the warnining is totally reasonable... People should have axes labels on their data including units. The more we can pester people into doing that (I think) the better, if it's really that annoying it's only a warning, you can silence it.

I think these warnings just get very annoying. I think it by default should set it to pixels or arbitrary units. To avoid the warning.

It might also be nice to make a small Example using a dummy test dataset to show how to do some of the more simple operations.

Agreed! We should be able to make nice dataset to show the DPC features.

…it can go through an appropriate deprecation cycle.

CSSFrancis · 2024-06-03T20:50:35Z

@magnunor I fixed the bug causing the tests to fail and adjusted some of the code. I think this is starting to look pretty good, the only thing I would like to look into is what other visualizations might be of interest.

We could think about things like:

Vector map overlays
Colormap overlays with a color key (for the navigator)
Stream plots (This one is a little more iffy)
Showing the direct beam shift as a circle for both shifted and the unshifted vector.

magnunor · 2024-06-04T06:34:56Z

@magnunor I fixed the bug causing the tests to fail and adjusted some of the code.

Very nice!

We could think about things like:

1. Vector map overlays

2. Colormap overlays with a color key (for the navigator)

3. Stream plots (This one is a little more iffy)

4. Showing the direct beam shift as a circle for both shifted and the  unshifted vector.

I agree that these would be very good. However, since these are additions, rather than API-breaking, we should wait with these for the next release.

CSSFrancis · 2024-06-04T10:36:22Z

@magnunor sounds good. I'm going to review the changes again. If you have time can you go over my additions?

Then we can merge.

magnunor · 2024-06-04T10:54:28Z

I'll try to get this done today, or most likely tomorrow.

CSSFrancis

Just some small comments of things we could do eventually.

CSSFrancis · 2024-05-29T00:52:46Z

pyxem/signals/beam_shift.py



 class BeamShift(Signal1D):
    """Signal class for working with shift of the direct beam."""

    _signal_type = "beam_shift"

-    def make_linear_plane(self, mask=None):
+    def get_linear_plane(self, mask=None, fit_corners=None):


Currently this doesn't work on 5 dimensions but we can wait on that for a later PR.

CSSFrancis · 2024-06-04T11:54:01Z

pyxem/signals/beam_shift.py

+        if fit_corners is not None:
+            mask = bst._get_corner_mask(self.isig[0], corner_size=fit_corners)
+


This could be more efficient. Right now this requires that you calculate the entire shifts when really we only need to calculate the corners.

CSSFrancis · 2024-06-04T11:56:42Z

pyxem/signals/beam_shift.py

+
+        """
+        # Rotate the phase by -30 degrees in the color "wheel", to get better
+        # visualization in the vertical and horizontal direction.


I adjusted this but before the rotation was always rotation=30

sivborg · 2024-06-04T14:24:11Z

Hello, I work quite a bit with this functionality, so I can take a gander at some parts tomorrow and the day after, at least!

sivborg · 2024-06-04T14:26:45Z

examples/processing/centering_the_zero_beam.py

@@ -23,7 +23,7 @@
 # In many instances the zero beam position will vary systematically with the scan position.
 # This can be corrected by fitting a linear plane to the zero beam position using the
 # :meth:`make_linear_plane` method.
-shifts.make_linear_plane()  # Making a linear plane to remove the systematic shift
+shifts.get_linear_plane()  # Making a linear plane to remove the systematic shift


The functionality is changed here so that the plane is returned. So should AFAIK be shifts = shifts.get_linear_plane()

sivborg · 2024-06-05T08:42:28Z

I looked into using old DPC signals with the new BeamShift. However, the conversion between the old DPC signals and BeamShift requires a transpose, which I am wondering if it's necessary? This is the code I'm running:

import pyxem as pxm
import hyperspy.api as hs
s = hs.load("old_dpc.hspy")
print(s)
print(pxm.signals.BeamShift(s))
print(pxm.signals.BeamShift(s.T))

which has the output:

WARNING | Hyperspy | `signal_type='dpc'` not understood. See `hs.print_known_signal_types()` for a list of installed signal types or https://github.com/hyperspy/hyperspy-extensions-list for the list of all hyperspy extensions providing signals. (hyperspy.io:743)
WARNING:hyperspy.io:`signal_type='dpc'` not understood. See `hs.print_known_signal_types()` for a list of installed signal types or https://github.com/hyperspy/hyperspy-extensions-list for the list of all hyperspy extensions providing signals.
<Signal2D, title: , dimensions: (2|256, 256)>
<BeamShift, title: , dimensions: (256, 2|256)>
WARNING | Hyperspy | `signal_type='dpc'` not understood. See `hs.print_known_signal_types()` for a list of installed signal types or https://github.com/hyperspy/hyperspy-extensions-list for the list of all hyperspy extensions providing signals. (hyperspy.io:743)
WARNING:hyperspy.io:`signal_type='dpc'` not understood. See `hs.print_known_signal_types()` for a list of installed signal types or https://github.com/hyperspy/hyperspy-extensions-list for the list of all hyperspy extensions providing signals.
<BeamShift, title: , dimensions: (256, 256|2)>

In the past you would use the to_beamshift method which would do this for you. But is it good to maybe consider doing it automatically when sending in a (2|nx, ny) signal instead of giving a (256, 2| 256) signal? That way old data is more compatible.

sivborg · 2024-06-05T09:47:30Z

pyxem/signals/beam_shift.py

+        s_rgb.change_dtype("rgb16")
+        return s_rgb
+
+    def get_color_signal(


Would it be a good idea to rename this function? As it can easily be confused with get_phase_signal since it's also very colourful. Maybe something like get_magnitude_phase_signal. Easier to search for too.

magnunor · 2024-06-05T10:55:10Z

In the past you would use the to_beamshift method which would do this for you. But is it good to maybe consider doing it automatically when sending in a (2|nx, ny) signal instead of giving a (256, 2| 256) signal? That way old data is more compatible.

This is a good point: how to best handle the old files. I'm not sure how to best do this, as we would like to drop the DPCSignal in HyperSpy extensions? Alternatively, we could keep a small placeholder class which has one function to convert the data into a BeamShift signal?

This could be more efficient. Right now this requires that you calculate the entire shifts when really we only need to calculate the corners.

This is a very good point! This should also be useful if different masks are used, essentially that we only calculate this for the chunks which we really need.

Currently this doesn't work on 5 dimensions but we can wait on that for a later PR.

Agreed, we should add a NotImplementedError for signals with the unsupported dimensions.

CSSFrancis · 2024-06-05T11:39:37Z

In the past you would use the to_beamshift method which would do this for you. But is it good to maybe consider doing it automatically when sending in a (2|nx, ny) signal instead of giving a (256, 2| 256) signal? That way old data is more compatible.

I think this is totally reasonable. We should keep the DPC class and signal type but just transpose the data and set the signal type to beam_shift for consistency. For things like loading data I think I'm okay for keeping deprecation warnings along for longer.

CSSFrancis · 2024-06-05T12:22:42Z

@magnunor If you don't have time to get to this today I might try to make things a bit more backwards compatible and then merge.

magnunor · 2024-06-05T13:00:33Z

Go for it! I can review it.

ericpre · 2024-06-05T13:29:08Z

To read files containing "dpc" signal type, the conversion can also be done in rosettasciio - in a similar way as markers are converted from hyperspy 1.x to 2.x for example. This has the advantage of not having to keep dpc signal for ever in pyxem.

magnunor · 2024-06-05T13:55:59Z

To read files containing "dpc" signal type, the conversion can also be done in rosettasciio - in a similar way as markers are converted from hyperspy 1.x to 2.x for example. This has the advantage of not having to keep dpc signal for ever in pyxem.

That sounds tempting. How would this work? Looking for the dpc signal type? What if someone else wants to use the dpc namespace in the future?

ericpre · 2024-06-05T14:22:30Z

There should be enough metadata (see for example https://github.com/hyperspy/rosettasciio/blob/31bd677cc4c02c5787ae9b61250bc1431f352cba/rsciio/_hierarchical.py#L309 and https://github.com/hyperspy/rosettasciio/blob/31bd677cc4c02c5787ae9b61250bc1431f352cba/rsciio/utils/tools.py#L466) that is saved in the file to figure out when it needs to be converted and not interfere with other "dpc" signal type.

Note that keeping an empty class in pyxem will prevent other use of this signal type.

Merge branch 'main' into restructure_dpc_functionality

2cc3da7

magnunor added 5 commits April 10, 2024 12:07

Remove DPCSignal classes and fix issues flowing from this

af95f62

Fix failing unit test due to DPCSignal to BeamShift change

cbab08a

Fix dummy_data docstrings: DPCSignal to BeamShift

bbdcdd2

Further major restructure of DPCSignal to BeamShift

3c79e64

- Make new _beam_shift_tools module - Remove Diffraction2D.center_of_mass: -- Move to get_direct_beam_position(method="center_of_mass") - Update many unit tests due to these changes - Update many docstrings due to these changes

Various style and linting fixes for the DPC restructuring

1749c72

magnunor mentioned this pull request Apr 21, 2024

Improving the Differential Phase Contrast processing structure #1039

Open

Add a preliminary changelog in CHANGELOG.rst

4642489

CSSFrancis mentioned this pull request Apr 21, 2024

Better Handling Masks #1063

Open

magnunor added 2 commits May 3, 2024 14:13

Switch BeamShift.plot over to using hyperspy.utils.plot.plot_images

c413986

Add check for lazy signal in BeamShift.plot

681e769

emichr mentioned this pull request May 27, 2024

Unexpected get_direct_beam_position behaviour with center_of_mass method. #1079

Open

CSSFrancis reviewed May 28, 2024

View reviewed changes

CSSFrancis added 2 commits May 28, 2024 09:48

Temp: Refactor beamshift

6b762bd

Refactor: BeamShift--> DiffractionVector1D

40b9db4

CSSFrancis mentioned this pull request May 29, 2024

0.19.0rc #1077

Open

3 tasks

Merge branch 'main' into restructure_dpc_functionality

5e2011c

CSSFrancis added 3 commits June 3, 2024 14:19

Testing: Fix failing Tests due to COM on Slices

a44b25b

Clean Up: Cleaning up some functions. Readded make_linear_plane so …

5190e77

…it can go through an appropriate deprecation cycle.

Examples: Add Example for Magnetic DPC.

01dd031

Testing: Fix regression

b9f881f

CSSFrancis approved these changes Jun 4, 2024

View reviewed changes

sivborg reviewed Jun 4, 2024

View reviewed changes

sivborg reviewed Jun 5, 2024

View reviewed changes

Improve the STEM-DPC example

8674538

		if fit_corners is not None:
		mask = bst._get_corner_mask(self.isig[0], corner_size=fit_corners)

Large restructure of differential phase contrast class structure #1057

Are you sure you want to change the base?

Large restructure of differential phase contrast class structure #1057

Conversation

magnunor commented Apr 7, 2024 • edited

coveralls commented Apr 7, 2024 • edited

CSSFrancis commented Apr 8, 2024

magnunor commented Apr 10, 2024

magnunor commented Apr 14, 2024 • edited

magnunor commented Apr 21, 2024 • edited

CSSFrancis commented Apr 21, 2024

magnunor commented May 3, 2024

CSSFrancis 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

CSSFrancis commented May 29, 2024

CSSFrancis commented May 29, 2024

magnunor commented May 29, 2024

CSSFrancis commented May 29, 2024

magnunor commented May 31, 2024

CSSFrancis commented May 31, 2024

CSSFrancis commented Jun 1, 2024

magnunor commented Jun 3, 2024

CSSFrancis commented Jun 3, 2024

magnunor commented Jun 4, 2024

CSSFrancis commented Jun 4, 2024

magnunor commented Jun 4, 2024

CSSFrancis 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

sivborg commented Jun 4, 2024

Choose a reason for hiding this comment

sivborg commented Jun 5, 2024 • edited

Choose a reason for hiding this comment

magnunor commented Jun 5, 2024

CSSFrancis commented Jun 5, 2024

CSSFrancis commented Jun 5, 2024

magnunor commented Jun 5, 2024

ericpre commented Jun 5, 2024

magnunor commented Jun 5, 2024

ericpre commented Jun 5, 2024

magnunor commented Apr 7, 2024 •

edited

coveralls commented Apr 7, 2024 •

edited

magnunor commented Apr 14, 2024 •

edited

magnunor commented Apr 21, 2024 •

edited

sivborg commented Jun 5, 2024 •

edited