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

align_zero_loss_peak should take left and right arguments for low loss spectra #7

Merged
merged 8 commits into from Nov 18, 2023
Merged

Conversation

HanHsuanWu
Copy link
Contributor

Since now exspy is split hyperspy/hyperspy#3249, I moved the PR here.

Description of the change
Adding additional description for align_zero_loss_peak and added left and right arguments.
When subpixel = True, align1D (with cross-correlation) is used. The range of align1D depends on variables "left" and "right."
Currently by default, left and right are set to be -3. and 3. I think these are in whichever unit the energy axis is in.
Thus, for low loss data that are in the units of meV being about to adjust them will be helpful.

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (987e6a8) 91.83% compared to head (1ab68ea) 88.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
- Coverage   91.83%   88.09%   -3.74%     
==========================================
  Files          67       67              
  Lines        7274     7284      +10     
  Branches        0     1175    +1175     
==========================================
- Hits         6680     6417     -263     
+ Misses        594      593       -1     
- Partials        0      274     +274     

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

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.

Indeed, it would be useful to add these as argument, however, it is slightly more complicated as it is currently done in this PR, in some situation it can be wrong. These argument are passed to align1D, which used usual hyperspy syntax: axis indices (when integers are passed) or axis values (with float). However, when using calibrate the argument will convert to float when adding the mean of the zero loss peak position, which then become wrong.

In this case, because of the calibrate option, it may be good to cast int to float to enforce the use of axis values as oppose to axis indices.

To be consistent with other functions, such as align1D, it may be more suitable to use start and end - however, it may be worth checking the syntax of other similar functions to figure out if one of the two wording is better.

It would also need some tests to cover the behaviour described above.

@HanHsuanWu
Copy link
Contributor Author

So I changed left and right to start and end. I think this way is more intuitive and consistent with align1D. Also I think just forcing them to be float is much easier. How do I add the tests?

@ericpre
Copy link
Member

ericpre commented Nov 2, 2023

There is some guidance at https://hyperspy.org/hyperspy-doc/dev/dev_guide/testing.html#writing-tests and you can have a look at other tests which are similar:

class TestAlignZLP:
def setup_method(self, method):
s = exspy.signals.EELSSpectrum(np.zeros((10, 100)))
self.scale = 0.1
self.offset = -2
eaxis = s.axes_manager.signal_axes[0]
eaxis.scale = self.scale
eaxis.offset = self.offset
self.izlp = eaxis.value2index(0)
self.bg = 2
self.ishifts = np.array([0, 4, 2, -2, 5, -2, -5, -9, -9, -8])
self.new_offset = self.offset - self.ishifts.min() * self.scale
s.data[np.arange(10), self.ishifts + self.izlp] = 10
s.data += self.bg
s.axes_manager[-1].offset += 100
self.signal = s
def test_align_zero_loss_peak_calibrate_true(self):
s = self.signal
s.align_zero_loss_peak(calibrate=True, print_stats=False)
zlpc = s.estimate_zero_loss_peak_centre()
np.testing.assert_allclose(zlpc.data.mean(), 0)
np.testing.assert_allclose(zlpc.data.std(), 0)
def test_align_zero_loss_peak_calibrate_true_with_mask(self):
s = self.signal
mask = s._get_navigation_signal(dtype="bool").T
mask.data[[3, 5]] = (True, True)
s.align_zero_loss_peak(calibrate=True, print_stats=False, mask=mask)
zlpc = s.estimate_zero_loss_peak_centre(mask=mask)
np.testing.assert_allclose(np.nanmean(zlpc.data), 0, atol=np.finfo(float).eps)
np.testing.assert_allclose(np.nanstd(zlpc.data), 0, atol=np.finfo(float).eps)
def test_align_zero_loss_peak_calibrate_false(self):
s = self.signal
s.align_zero_loss_peak(calibrate=False, print_stats=False)
zlpc = s.estimate_zero_loss_peak_centre()
np.testing.assert_allclose(zlpc.data.std(), 0, atol=10e-3)
def test_also_aligns(self):
s = self.signal
s2 = s.deepcopy()
s.align_zero_loss_peak(calibrate=True, print_stats=False, also_align=[s2])
zlpc = s2.estimate_zero_loss_peak_centre()
assert zlpc.data.mean() == 0
assert zlpc.data.std() == 0
def test_align_zero_loss_peak_with_spike_signal_range(self):
s = self.signal
spike = np.zeros((10, 100))
spike_amplitude = 20
spike[:, 75] = spike_amplitude
s.data += spike
s.align_zero_loss_peak(
print_stats=False, subpixel=False, signal_range=(98.0, 102.0)
)
zlp_max = s.isig[-0.5:0.5].max(-1).data
# Max value in the original spectrum is 12, but due to the aligning
# the peak is split between two different channels. So 8 is the
# maximum value for the aligned spectrum
np.testing.assert_allclose(zlp_max, 8)
def test_align_zero_loss_peak_crop_false(self):
s = self.signal
original_size = s.axes_manager.signal_axes[0].size
s.align_zero_loss_peak(crop=False, print_stats=False)
assert original_size == s.axes_manager.signal_axes[0].size

Typically, it is a matter to add tests against cases that cover the expected behaviour.

@HanHsuanWu
Copy link
Contributor Author

@ericpre I am not exactly sure if this is what you mean but I added two cases to make sure the function works when start and end inputs are floats and ints. In the function both start and end should be enforced as float.

@ericpre ericpre merged commit add7440 into hyperspy:main Nov 18, 2023
16 of 17 checks passed
@ericpre
Copy link
Member

ericpre commented Nov 18, 2023

@HanHsuanWu, thank you for your contribution. I push a few commits to do the following:

  • fix tests, which were failing
  • use signal_range instead of adding start and end argument. When reviewing the PR, I notice this argument is used to specify the start and end but only for subpixel=False and adding other parameter to do the same thing should be avoided following https://peps.python.org/pep-0020.
  • add changelog entry

You can go through the commits to follow the various steps in case you are interested in understanding more the details.

@HanHsuanWu
Copy link
Contributor Author

Thank you @ericpre. I appreciated the help very much!

@ericpre ericpre added this to the v0.1 milestone Dec 2, 2023
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

2 participants