LumiSpy / lumispy

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

reorganize transient classes #118

Closed jlaehne closed 2 years ago

jlaehne commented 2 years ago

Description of the change

Closes #115

Split Transient classes into 1D Transient and 2D TransientSpectrum classes. Add CommonTransient class for functionality that will work on the time axis.

In a separate PR, functionalities of LuminescenceSpectrum class that are important also for streak camera data, will need to be moved to CommonLuminescence.

Progress of the PR

codecov-commenter commented 2 years ago

Codecov Report

Merging #118 (711b236) into main (afd9723) will increase coverage by 0.23%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   95.76%   96.00%   +0.23%     
==========================================
  Files          11       15       +4     
  Lines         591      626      +35     
==========================================
+ Hits          566      601      +35     
  Misses         25       25              
Impacted Files Coverage Δ
lumispy/signals/cl_transient.py 100.00% <100.00%> (ø)
lumispy/signals/cl_transientspec.py 100.00% <100.00%> (ø)
lumispy/signals/common_transient.py 100.00% <100.00%> (ø)
lumispy/signals/luminescence_transient.py 100.00% <100.00%> (ø)
lumispy/signals/luminescence_transientspec.py 100.00% <100.00%> (ø)
lumispy/signals/pl_transient.py 100.00% <100.00%> (ø)
lumispy/signals/pl_transientspec.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update afd9723...711b236. Read the comment docs.

jordiferrero commented 2 years ago

Thanks Jonas! Indeed, nobody has really used these classes quite yet (I am working on some functions for the PL transient 1D.

My only comment, following what @LMSC-NTappy said, is that I would rather keep the names close to the signal features and not the detector. So what about calling it cl_transient_1d or cl_transient_2d instead of transient and streak? Maybe I am getting confused here, but streak implies a streak camera, right? Is there a way of getting 2d luminescence data with other detectors? Idk

jlaehne commented 2 years ago

Well, these are 'just' the file names and not the class names, which are e.g. TransientSpec, but I could do the same and use transientspec in the file names.

I'm not aware of any other method to obtain spectrally resolved time traces.

jlaehne commented 2 years ago

Only suggetion: I think the file in the signals/ directory could be changed to reflect the new class names.

Done in https://github.com/LumiSpy/lumispy/pull/118/commits/711b236fb1007a83841fe08079315fe60bb0319c

jordiferrero commented 2 years ago

Thanks Eric. Just to put this into context, we (I recall Duncan and me) created all these classes at the very beginning of LumiSpy to make sure we had a structured "vision" of what the package should look like. I agree no one has had time to dive deeper in the transient classes yet. So I am happy to clean up the package as I think the vision is clear :-)