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

reorganize transient classes #118

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented Apr 9, 2022

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

  • Change implemented (can be split into several points),
  • docstring updated (if appropriate),
  • added tests,
  • added line to CHANGELOG.md,
  • ready for review.

@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022

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

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

jlaehne commented Apr 10, 2022 via email

Copy link
Contributor

@LMSC-NTappy LMSC-NTappy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Regarding time-resolved luminescence, I think it can be done using TCSPC and serial spectral acquisition, gated detectors or regular detectors and pump-probe experiment schemes. Not sure what is the dominant

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

Cheers

Nicolas

@jlaehne jlaehne added this to the v0.2.0 milestone Apr 21, 2022
@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 21, 2022

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

Done in 711b236

@jlaehne jlaehne mentioned this pull request Apr 21, 2022
@ericpre ericpre mentioned this pull request Apr 23, 2022
Copy link
Contributor

@ericpre ericpre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back to the original discussion and add a comments in #5 (comment).
Based on this comment, I would simply suggest to remove the transient classes, for the following reasons:

  • It is not clear, what they will be used for
  • we should consider them when the need arise
  • these signal_type can be added later without affecting the existing structure. Actually, already defining a structure could be annoying in the future, in case they needs to be changed!

@ericpre ericpre merged commit 72b18e4 into LumiSpy:main Apr 27, 2022
@jordiferrero
Copy link
Contributor

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 :-)

@jlaehne jlaehne deleted the transient-signal-classes branch April 29, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transient signal classes
5 participants