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

Class hierarchy #5

Closed
dnjohnstone opened this issue Nov 19, 2019 · 13 comments · Fixed by #67
Closed

Class hierarchy #5

dnjohnstone opened this issue Nov 19, 2019 · 13 comments · Fixed by #67

Comments

@dnjohnstone
Copy link
Contributor

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
Copy link
Contributor

jlaehne commented Nov 19, 2019

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
Copy link
Contributor

jordiferrero commented Nov 20, 2019

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

@jlaehne
Copy link
Contributor

jlaehne commented Nov 21, 2019

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

@dnjohnstone
Copy link
Contributor Author

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
Copy link
Contributor

jlaehne commented Nov 25, 2019

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
Copy link
Contributor

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.

@jordiferrero jordiferrero moved this from In progress to Done in Basic functions for first release Apr 21, 2020
@jordiferrero jordiferrero moved this from Done to In progress in Basic functions for first release Apr 21, 2020
@jordiferrero jordiferrero moved this from In progress to Done in Basic functions for first release Apr 21, 2020
@jlaehne
Copy link
Contributor

jlaehne commented Apr 21, 2020

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

  • cosmic_rays_subtraction is necessary for any type of spectrum recorded with a CCD, so I would move it to CommonLumi or LumiSpectrum (depending on whether it plays a role also for streak camera data or not, I'm not sure)
  • I am not so sure about the correct position for correct_grating_shift. It looks to me like it needs specific Attolight metadata. That should be mentioned at least in the docstring. Or would it make sense to rework it to a more general function? If there will be more Attolight specific functions, we might have to introduce an Attolight specific sub-signal.

@jordiferrero
Copy link
Contributor

I think this can be closed.

@jlaehne jlaehne mentioned this issue Mar 28, 2021
4 tasks
@jlaehne jlaehne linked a pull request Mar 28, 2021 that will close this issue
4 tasks
@jlaehne jlaehne moved this from In progress to Done in LumiSpy first release 0.1 Mar 31, 2021
@ericpre
Copy link
Contributor

ericpre commented Apr 23, 2022

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>.

  • the way we structure our data analysis is independent of how the data have been acquired. The fact that the streak camera generate an image with one axis being a time axis is not a reason for processing data with the time as signal axis, in my opinion. When reading the data, we need to set the axis so that the data is structure in a way for analysis, regardless of how it has been acquired.

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
Copy link
Contributor

jlaehne commented Apr 24, 2022

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
Copy link
Contributor

ericpre commented Apr 26, 2022

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
Copy link
Contributor

jlaehne commented Apr 26, 2022 via email

@ericpre
Copy link
Contributor

ericpre commented Apr 27, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants