-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix hamamatsu signal type #209
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
=======================================
Coverage 87.81% 87.81%
=======================================
Files 83 83
Lines 11155 11155
Branches 2280 2280
=======================================
Hits 9796 9796
Misses 860 860
Partials 499 499 ☔ View full report in Codecov by Sentry. |
rsciio/hamamatsu/_api.py
Outdated
@@ -359,7 +359,7 @@ def _map_signal_md(self): | |||
) | |||
signal["signal_type"] = "" | |||
else: | |||
signal["signal_type"] = "Luminescence" # pragma: no cover | |||
signal["signal_type"] = "TransientSpec" # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lumispy implementation details, but wouldn't be better to have "Transcient" as signal_type
and use the signal_dimension
to map to the correct class depending if there is an energy axis or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The streak camera gives images with one axis being wavelength and one time, so this reader should not come across 1D signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was not clear: I was suggesting to use Transient
here in order to use it for both the LumiTransient
and LumiTransientSpectrum
class, which would require changes in lumispy and would fix behaviour reported in LumiSpy/lumispy#204: when changing signal_dimension
, hyperspy will map to the correct class.
I have just asked myself the same questions as in LumiSpy/lumispy#5 (in particular, does it make sense to have a signal containing dimension with signal type of different nature (energy/wavelength versus time)?
If the discussion on the structure of transient signal can be resolved for good soon, should we wait for the relevant changes in lumispy and update accordingly this PR? If not, we can merge this PR, but I imagine that it will need to be changed when the structure of the transient classes are fixed (currently, I don't think that it is fully compatible with the hyperspy signals paradigm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, my misunderstanding was that signal_type
is directly linked to the signal class. But a Signal2D can actually have a signal type from a 1D class.
For the moment it would be the quickest fix to directly set LumiTransientSpectrum
to allow for the energy conversion, but from what you outline, it could also be done downstream by discovering the signal_dimension and then using the correct LumiTransientSpectrum
class? So currently, there is no added value of using Transient
here instead of Luminescence
, but in the long run, there should be dedicated functionalities for the two different signal types combined in one class.
Thus, to resolve this issue it would be a possibility to allow for a list of signal_type
upstream in hyperspy, where the order determines what axes the signal types correspond to.
Adapted to changes in LumiSpy/lumispy#205 |
Description of the change
The hamamatsu reader was trying to set the
signal_type
to theSignal1D
subclassLuminescence
instead of theSignal2D
subclassTransientSpec
whenlumispy
is available.Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),docs/readthedocs.org:rosettasciio
build of this PR (link in github checks)