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

Utility to denoise laser profile and adding a test for the same #315

Open
wants to merge 45 commits into
base: development
Choose a base branch
from

Conversation

Paaaaarth
Copy link
Contributor

@Paaaaarth Paaaaarth commented Oct 23, 2024

MERGE AFTER #341 .

Denoising can be considered as one of the common thing that's done while creating a laser object and yet there is no trivial way to do so in LASY. With this pull request we aim to add this functionality i.e. mode decomposition based denoising. In its current state the code does two distinct things, mode decomposition and creating denoised profile.

Copy link
Contributor

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! A few general points:

  1. Good idea to put this in a separate file denoise.py as you propose, if we consider having other denoising methods implemented later. Could you adapt the naming etc.? For instance, the function could be called: denoise_LG_modes or so.
  2. I would encourage you to check mode_decomposition.py, it can take either a full Laser profile or a Transverse profile, and explain what it does in each case. IMO the clearest would be for your function to take a generic profile that can be a TransverseProfile or a Laser but always return a TransverseProfile. In the future, we could always return the same type as input, I think that would be the best, but would take more work to handle the longitudinal data.
  3. Could you avoid ambiguous input arguments like parameters bundling different variables, unless necessary? Just pass the arguments you need, and no more (for instance you may get rid of polarization).
  4. Please document all input parameters, check other functions for example.
  5. Please add a CI test.

@Paaaaarth Paaaaarth changed the title [WIP] Utility to denoise laser profile [WIP] Utility to denoise laser profile and adding a test for the same Oct 28, 2024
@Paaaaarth Paaaaarth marked this pull request as draft October 29, 2024 12:54
lasy/utils/denoise.py Fixed Show fixed Hide fixed
tests/test_denoise.py Fixed Show fixed Hide fixed
@Paaaaarth Paaaaarth changed the title [WIP] Utility to denoise laser profile and adding a test for the same Utility to denoise laser profile and adding a test for the same Nov 19, 2024
lasy/utils/denoise.py Fixed Show fixed Hide fixed
@Paaaaarth
Copy link
Contributor Author

Paaaaarth commented Nov 22, 2024

With D4, We have added the additional capability of inputing an image file as transverse profile. Skimage package is used to extract the intensity data.

lasy/utils/denoise.py Fixed Show fixed Hide fixed
tests/test_denoise.py Fixed Show fixed Hide fixed
lasy/utils/denoise.py Fixed Show fixed Hide fixed
@rob-shalloo
Copy link
Member

Hey @Paaaaarth thanks for the PR! This is great to have the functionality in one place. I would make a few suggestions. Feel free to take or leave as you see fit.

  1. While this is a great technique to denoise laser profiles, I actually think this goes much beyond just denoising. We're really talking here about creating an analytic representation of the laser pulse (as a sum of HG modes) from an arbitrary input profile. I would then suggest that you change the name to something like hg_reconstruction. We could of course collect such methods as Maxence suggest like lg_reconstruction in one place within the documentation to showcase their use for denoising.

  2. I would suggest removing the creation of a transverse_profile_from_data here and leave that to the user. Ie have an assert that you only accept existing transverse profiles or again as Maxence suggest full laser profiles could be interesting in the future. I think the transverse_profile_from_data complicates things as it would not be so generic. You in any case do not allow the user to select how the raw image is processed, for example, the threshold at which you set pixel values to 0 (here it appears to be anything less than 1%). This always takes some tuning depending on the camera model, bit depth, background subtraction, signal strength etc.

  3. This is probably a larger 'fix' but I would also suggest we allow different values of the waist in x and y and actually that in addition to providing some functionality to "guess" the values, we allow the user to specify values. A sensible choice of waist can quite often speed up the decomposition / reconstruction process. This would mean digging into hermite_gauss_decomposition a bit.

Copy link
Contributor

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! The core of it looks good, see some comments below.
As per Rob's points:

  1. the naming: sounds good to me, I don't do denoising etc. on a daily basis.
  2. agreed, I think I have a similar comment in a comment below.
  3. Indeed the option of having 2 waists would be nice. Can be done in a follow-up PR.

lasy/utils/denoise.py Outdated Show resolved Hide resolved
lasy/utils/denoise.py Outdated Show resolved Hide resolved
lasy/utils/denoise.py Outdated Show resolved Hide resolved
lasy/utils/denoise.py Outdated Show resolved Hide resolved
lasy/utils/denoise.py Show resolved Hide resolved
lasy/utils/denoise.py Outdated Show resolved Hide resolved
tests/test_denoise.py Fixed Show fixed Hide fixed
tests/test_denoise.py Fixed Show fixed Hide fixed
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.

3 participants