LumiSpy / lumispy

Luminescence data analysis with HyperSpy.
https://lumispy.org
GNU General Public License v3.0
26 stars 18 forks source link

Class hierarchy #5

Closed dnjohnstone closed 3 years ago

dnjohnstone commented 4 years ago

An issue to discuss openly class hierarchy for lumispy, from an e-mail with @jlaehne

We want to start out with CL, but need a more general parent that allows to add PL and EL at some point. Also you probably want to add TR-CL/PL at some point.

In general, I guess something like

-lumispec (inheriting from hyperspy/signal1D) |-cl |-cl-sem |-cl-stem |-pl |-el -trlumi (inheriting from hyperspy/signal2D ?) |-trcl |-trpl

jlaehne commented 4 years ago

Well cl-sem and cl-stem would inherit from cl, and sticking to the naming of related hyperspy signals, that would give e.g. (with aliases in [ ]):

-LumiSpectrum (inheriting from hyperspy/signal1D) [LuminescenceSpectrum] |-CLSpectrum [CL,cathodoluminescence] ||-CLSEMSpectrum [CLSEM] ||-CLSTEMSpectrum [CLSTEM] |-PLSpectrum [PL] |-ELSpectrum [EL] -LumiTransient (inheriting from hyperspy/signal2D ?) [or TRLumi] |-CLTransient [TRCL] |-PLTransient [TRPL]

Transient could also be replaced by TimeRes. What would you prefer?

We cans till define several aliases, such

jordiferrero commented 4 years ago

I will work on the hierarchy and I personally like the name TimeRes more than Transient.

jlaehne commented 4 years ago

I thought that CLTransient is more similar to CLSpectrum, with TR... and possibly also TimeResolved... as aliases.

dnjohnstone commented 4 years ago

I'm a little confused about the time resolved stuff and dimensionality... That's probably because I don't do it!

Why is time-resolved stuff not <t|E> i.e. a Signal1D with a time navigation axis?

Do you do time and spatially resolved stuff? E.g. <x,y|t,E>?

To move forward in a sensible way. I think we'll establish the code hierarchy for all the non-time-resolved stuff now, and make the 0.0.1 release and then we can add the time resolved stuff when ready.

jlaehne commented 4 years ago

Well that is why I put it to discussion. Is the time an additional navigation or signal axis? In principle, I would say it could be both and we have to find the most suitable convention.

You can still have 0, 1 or 2 spatial navigation dimensions. In TRPL spatial resolution is indeed uncommon (not impossible), but that is one of the main points of doing TRCL. So, if time becomes a navigation axis, it would have to become the first navigation dimension [0], to be independent of how many spatial dimensions there are.

Of course, time is not a signal axis in the sense of e.g. a STEM hologram or diffraction pattern, where you have two signal axes of the same type. However, it is also not of the same type as the spatial navigation dimensions.

A streak camera collects an image of time versus wavelength <t,E> and when you do a map you get <x,y|t,E>. So, in that way, it really makes most sense to define time as additional signal axis. You might want to extract either spectra at a specific delay <x,y|E>, or time-traces (transients) for a specific wavelength window <x,y|t>. In fact, for other acquisition modes, such as time-correlated single photon counting, you actually do not have a wavelength axis and time remains the only signal axis <x,y|t>. In the end, an essential feature will be to fit models to these transients.

So thinking about it, time should indeed be a signal axis!

jordiferrero commented 4 years ago

I have added all these changes now. I have left the LumiTransient (Singnal 2D) class open to any subclass. I guess it will make sense to add them in the future. But for now, it is more tidy and similar to Hyperspy. I have also added a supra-class called CommonLumi (similar to how pyxem deals with it) to mimic the role of BaseSignal in Hyperspy. In this general class I have added some functions that apply to both 1D and 2D signals (like modifing the navigation axis).

If you are happy with it, I will close this issue.

jlaehne commented 4 years ago

Just minor points about the functions defined in the different classes:

jordiferrero commented 3 years ago

I think this can be closed.

ericpre commented 2 years ago

Well that is why I put it to discussion. Is the time an additional navigation or signal axis? In principle, I would say it could be both and we have to find the most suitable convention.

You can still have 0, 1 or 2 spatial navigation dimensions. In TRPL spatial resolution is indeed uncommon (not impossible), but that is one of the main points of doing TRCL. So, if time becomes a navigation axis, it would have to become the first navigation dimension [0], to be independent of how many spatial dimensions there are.

Of course, time is not a signal axis in the sense of e.g. a STEM hologram or diffraction pattern, where you have two signal axes of the same type. However, it is also not of the same type as the spatial navigation dimensions.

Reading this while reviewing #118 and I am not convinced that there is a case for having so many classes for the following reasons:

A streak camera collects an image of time versus wavelength <t,E> and when you do a map you get <x,y|t,E>. So, in that way, it really makes most sense to define time as additional signal axis. You might want to extract either spectra at a specific delay <x,y|E>, or time-traces (transients) for a specific wavelength window <x,y|t>.

In fact, for other acquisition modes, such as time-correlated single photon counting, you actually do not have a wavelength axis and time remains the only signal axis <x,y|t>. In the end, an essential feature will be to fit models to these transients.

  • I don't see why it needs a specific class and signal_type, as this can achieve transposing axes and a generic signal. To give a similar example in a different context, you could model a composition obtained from a EDS linescan across an interface or core-shell nanoparticles, etc. and fits it. The same approach could be used (transposition with a generic signal and fitting) without creating EDS linescan signal type.

These classes are empty at the moment, I would suggest to remove them and only consider creating such signal_type when the need arise.

jlaehne commented 2 years ago

Well, I envision e.g. having some specific models for time resolved data in these classes. Though the additional CL/PL subclasses are maybe overkill at the moment.

ericpre commented 2 years ago

Well, I envision e.g. having some specific models for time resolved data in these classes. Though the additional CL/PL subclasses are maybe overkill at the moment.

It is worth creating these classes only when the needs arise? Since, it is not defined what the functionalities would entail, what the requirement would be and there is ambiguity on its structure, I don't think that we should plan/speculate on how it should be. At least I can't do it without further information and I am not convinced that this is a efficient use of our time to do this now.

jlaehne commented 2 years ago

It is worth creating these classes only when the needs arise? Since, it is not defined what the functionalities would entail, what the requirement would be and there is ambiguity on its structure, I don't think that we should plan/speculate on how it should be. At least I can't do it without further information and I am not convinced that this is a efficient use of our time to do this now.

I think need will arise soon. We had already introduced a transient class - and realized that it was a too quick shot, which this PR is supposed to correct (in that sense your comment is correct).

To able to use the energy axis facilities of lumispy for streak camera images, we will have to have a 2D signal class that inherits from the 1D luminescence classes. Similarly, it should inherit from a class that provides time-specific functionalities. So I'm convinced that this is the right way to go.

ericpre commented 2 years ago

To able to use the energy axis facilities of lumispy for streak camera images, we will have to have a 2D signal class that inherits from the 1D luminescence classes. Similarly, it should inherit from a class that provides time-specific functionalities. So I'm convinced that this is the right way to go.

Ok, it seems that it would be need a bit of thinking!