Axis conversion for TransientSpec #205

jlaehne · 2024-01-01T23:24:49Z

Description of the change

Move axis conversion code to CommonLuminescence class to make it available to LuminescenceTransientSpectrum signals.

Handle inheritance for slicing of LuminescenceTransientSpectrum:

Create 1D version of signal_type = 'TransientSpec' (class TransientSpectrumCasting)
If TransientSpec is initialized in 1D with axes_manager[-1].units being a time unit, switch to LumiTransient class, otherwise switch to Luminescence class

Progress of the PR

move axis conversion code,
Implement workaround for Set axis replaces wrong axis for signal dimension > 1 hyperspy/hyperspy#3294
Handle slicing of TransientSpec,
add section to user guide,
add tests for TransientSpec case,
added line to CHANGELOG.rst,
check formatting of changelog and user guid,
ready for review.

codecov-commenter · 2024-01-01T23:26:07Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 99.29%. Comparing base (9c402c0) to head (ecf514f).
Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lumispy/signals/common_luminescence.py	97.56%	2 Missing ⚠️
lumispy/signals/luminescence_transient.py	50.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #205      +/-   ##
===========================================
- Coverage   100.00%   99.29%   -0.71%     
===========================================
  Files           12       12              
  Lines          556      567      +11     
===========================================
+ Hits           556      563       +7     
- Misses           0        4       +4

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

lumispy/signals/common_luminescence.py

+        >>> S1.to_invcm(laser=325)
+    """
+
+    def to_invcm_relative(self, laser=None, inplace=True, jacobian=False):


lumispy/tests/utils/test_axis_conversion.py

lumispy/tests/signals/test_luminescence_transient_spectrum.py

+# along with LumiSpy. If not, see <https://www.gnu.org/licenses/#GPL>.
+
+import numpy as np
+import pytest


lumispy/tests/signals/test_luminescence_transient_spectrum.py

+import pytest
+
+from lumispy.signals import LumiSpectrum, LumiTransient, LumiTransientSpectrum
+from hyperspy._signals.signal2d import Signal2D


lumispy/tests/signals/test_luminescence_transient_spectrum.py

+
+from lumispy.signals import LumiSpectrum, LumiTransient, LumiTransientSpectrum
+from hyperspy._signals.signal2d import Signal2D
+from numpy.testing import assert_allclose


lumispy/signals/__init__.py

ericpre

I don't like the idea of introducing the concept of "hidden signal" because by design registered signal class are not hidden.

If I understand correctly, the purpose of the TransientSpectrumCasting class is to assign to the correct class depending on the units of the last axis when converting to 1D signal. If so would it make sense to move this code to LumiTransient.__init__ and change to the Luminescence signal type when necessary? It should work if the TransientSpec signal type was added as an alias of the LumiTransient class?

lumispy/signals/luminescence_transientspec.py

lumispy/hyperspy_extension.yaml

lumispy/signals/luminescence_transientspec.py

jlaehne · 2024-10-14T21:57:57Z

@ericpre I was also hesitating to introduce a 'hidden signal class', but it works like a charm - while I cannot make it work without.

It is more elegant than what I proposed in #204 (comment) (and did not work), so I proposed this solution in #204 (comment)

I tried again having the initialization in the __init__ of one of the two existing 1D classes, but it then does not properly convert to the other. Instead, I have improved the documentation as you proposed.

jlaehne · 2024-10-15T08:58:40Z

@ericpre do you have an idea why the changelog build displays a numba warning? See: https://lumispy--205.org.readthedocs.build/en/205/changelog.html
Do I have to add that dependency to the workflow now that it is optional upstream?

Also, any idea on the Azure Pipelines failures?

ericpre · 2024-10-15T13:02:23Z

@ericpre do you have an idea why the changelog build displays a numba warning? See: https://lumispy--205.org.readthedocs.build/en/205/changelog.html Do I have to add that dependency to the workflow now that it is optional upstream?

This is surprising that it appears there! Yes, adding numba as dependencies or using hyperspy[speed] can be used a workaround.

Also, any idea on the Azure Pipelines failures?

mambaforge is deprecated, I will update what needs to be updated later today! 😉

jlaehne · 2024-10-17T10:36:42Z

@ericpre Do you want to have another look.

ericpre

This looks good, I have some comments to improve the documentation.

import lumispy as lum
import numpy as np

data = np.arange(10*20).reshape(10, 20)

s = lum.signals.LumiTransientSpectrum(data)
s.axes_manager[-1].units = "ns"
s.axes_manager[-2].units = "nm"

s.sum(-1)
s.sum(-2)

conda_environment_dev.yml

doc/user_guide/signal_axis.rst

doc/user_guide/streak_images.rst

ericpre · 2024-10-19T10:05:18Z

doc/user_guide/streak_images.rst

+
+.. image:: images/streakmap.svg
+  :width: 700
+  :alt: Plot of a streak camera image and its one-dimensional representations


Is there code in lumispy that can make this figure? When I do lumispy.signals.luminescence_transientspec.LumiTransientSpectrum.plot, I get a normal plot.

It would be good to add the code to make this figure, otherwise it should be removed?

I had previously prepared this figure for illustration purposes and thought it could be useful here to show the principle. It is indeed a useful feature to generally faciliate such kind of plots and I will open an issue to implement that in a separate PR (which could probably be taken on by one of my students). Not as the standard plotting of this class, but as a separate plot function (alternative to using sum an option to display spectra/transients for a certain range (or even an interactive span functionality can then be added later).

ericpre · 2024-10-19T10:06:56Z

lumispy/signals/common_luminescence.py

@@ -214,3 +224,300 @@ def normalize(self, pos=float("nan"), element_wise=False, inplace=False):
                s.metadata.Signal.quantity = "Normalized intensity"
        if not inplace:
            return s
+
+    def _reset_variance_linear_model(self):


Am I understanding correctly that this is simply moved around and doesn't need reviewed of the code itself?

Yes, all the code pertaining to signal axis conversion is moved to the CommonLuminescence class so that it is available also to LumiTransientSpectrum. That was the origin of this PR and I probably should have merged that moving part in before doing the other changes to make it clearer.

upcoming_changes/205.new.rst

jlaehne · 2024-10-21T12:16:14Z

This looks good, I have some comments to improve the documentation.

I implemented your suggestions (and extended some to the rest of the documentation). Only the plot creation will need a separate PR.

make axis conversion available to LumiTransientSpec

39deada

jlaehne added the type: enhancement label Jan 1, 2024

jlaehne added this to the v0.2.3 milestone Jan 1, 2024

github-advanced-security bot found potential problems Jan 1, 2024

View reviewed changes

workaround energy conversion

7927533

jlaehne force-pushed the common-lumi-code branch from 1f99632 to 7927533 Compare January 2, 2024 08:27

add test 2d axis conv

c251da6

github-advanced-security bot found potential problems Jan 27, 2024

View reviewed changes

lumispy/tests/utils/test_axis_conversion.py Dismissed Show dismissed Hide dismissed

jlaehne force-pushed the common-lumi-code branch from c5c1ee9 to 20c32b3 Compare January 27, 2024 12:07

sort out LumiTransientSpec inheritance

0d2c439

jlaehne force-pushed the common-lumi-code branch from 3b2fc4b to 0d2c439 Compare January 27, 2024 12:20

jlaehne mentioned this pull request Jan 27, 2024

Slicing TransientSpec #204

Open

test slicing

330cc47

jlaehne force-pushed the common-lumi-code branch from 0a0e021 to 330cc47 Compare January 27, 2024 13:20

github-advanced-security bot found potential problems Jan 27, 2024

View reviewed changes

github-advanced-security bot found potential problems Jan 28, 2024

View reviewed changes

lumispy/signals/__init__.py Fixed Show fixed Hide fixed

TransientSpec 1D class

9b54af6

jlaehne force-pushed the common-lumi-code branch from 5882277 to 9b54af6 Compare January 28, 2024 22:34

hide 1D TransientSpec

0132889

jlaehne linked an issue Jan 29, 2024 that may be closed by this pull request

Slicing TransientSpec #204

Open

jlaehne added 2 commits September 8, 2024 17:50

Merge remote-tracking branch 'upstream/main' into common-lumi-code

8ae9003

Merge remote-tracking branch 'upstream/main' into common-lumi-code

88d1c3d

ericpre reviewed Sep 21, 2024

View reviewed changes

jlaehne added 2 commits October 15, 2024 00:08

implement suggestions from code review

731df23

fix code scanning check notes

87551a9

jlaehne force-pushed the common-lumi-code branch from 42807f8 to 87551a9 Compare October 14, 2024 22:09

add changelog entry

8fa1413

jlaehne force-pushed the common-lumi-code branch from c91e1b4 to 69cc43a Compare October 15, 2024 07:57

jlaehne force-pushed the common-lumi-code branch from 69cc43a to 2d64d1a Compare October 15, 2024 08:19

add user_guide chapter

ae4def7

jlaehne force-pushed the common-lumi-code branch from 2d64d1a to ae4def7 Compare October 15, 2024 08:24

jlaehne added 2 commits October 16, 2024 08:14

Merge remote-tracking branch 'upstream/main' into common-lumi-code

b988e9c

change doc dependencies to include numba

fe8345b

jlaehne force-pushed the common-lumi-code branch from b7911a4 to fe8345b Compare October 16, 2024 06:38

jlaehne added the status: needs review label Oct 16, 2024

ericpre reviewed Oct 19, 2024

View reviewed changes

jlaehne mentioned this pull request Oct 21, 2024

Add plot function for streak images #217

Open

implement suggestions from code review

ecf514f

jlaehne mentioned this pull request Oct 21, 2024

Fix hamamatsu signal type hyperspy/rosettasciio#209

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Axis conversion for TransientSpec #205

Axis conversion for TransientSpec #205

jlaehne commented Jan 1, 2024 •

edited

Loading

codecov-commenter commented Jan 1, 2024 •

edited

Loading

ericpre left a comment

jlaehne commented Oct 14, 2024

jlaehne commented Oct 15, 2024 •

edited

Loading

ericpre commented Oct 15, 2024

jlaehne commented Oct 17, 2024

ericpre left a comment

ericpre Oct 19, 2024

jlaehne Oct 21, 2024

ericpre Oct 19, 2024

jlaehne Oct 21, 2024

jlaehne commented Oct 21, 2024

Axis conversion for TransientSpec #205

Are you sure you want to change the base?

Axis conversion for TransientSpec #205

Conversation

jlaehne commented Jan 1, 2024 • edited Loading

Description of the change

Progress of the PR

codecov-commenter commented Jan 1, 2024 • edited Loading

Codecov Report

ericpre left a comment

Choose a reason for hiding this comment

jlaehne commented Oct 14, 2024

jlaehne commented Oct 15, 2024 • edited Loading

ericpre commented Oct 15, 2024

jlaehne commented Oct 17, 2024

ericpre left a comment

Choose a reason for hiding this comment

ericpre Oct 19, 2024

Choose a reason for hiding this comment

jlaehne Oct 21, 2024

Choose a reason for hiding this comment

ericpre Oct 19, 2024

Choose a reason for hiding this comment

jlaehne Oct 21, 2024

Choose a reason for hiding this comment

jlaehne commented Oct 21, 2024

jlaehne commented Jan 1, 2024 •

edited

Loading

codecov-commenter commented Jan 1, 2024 •

edited

Loading

jlaehne commented Oct 15, 2024 •

edited

Loading