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.

Sign up for GitHub

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

Jump to bottom

RamanShiftCorrectionFeature find_peaks FWHM #133

Closed
mzieg opened this issue Apr 26, 2023 · 3 comments · Fixed by #289
Closed

RamanShiftCorrectionFeature find_peaks FWHM #133

mzieg opened this issue Apr 26, 2023 · 3 comments · Fixed by #289
Labels
1 - broken a userflow is impossible because something is broken prioritize There are reasons to do this sooner rather than later

Comments

@mzieg
Copy link
Member

mzieg commented Apr 26, 2023

The 'width' parameter in RamanShiftCorrectionFeature's call to scipy.signal.find_peaks should probably be tied to Spectrometer.fwhm rather than a hardcoded constant for all spectrometers. (The current implementation has apparently been found to work on all tested units, but it still seems like a design weakness.)
@samiebee43 samiebee43 added 5 - reimplement a self contained component could be made better and removed enhancement labels May 15, 2023
@mzieg
Copy link
Member Author

mzieg commented Jun 29, 2023

Internal user reported that RamanShiftCorrection was failing on some X-Series units during characterization...this could well be related. Bumping priority.

@mzieg mzieg added 1 - broken a userflow is impossible because something is broken and removed 5 - reimplement a self contained component could be made better labels Jun 29, 2023
@mzieg mzieg added the prioritize There are reasons to do this sooner rather than later label Jul 10, 2023
@samiebee43
Copy link
Contributor

Can we get more info about how RamanShiftCorrection is failing?

@samiebee43
Copy link
Contributor

And can we make this into a separate issue? You can link them by referencing the issue number. That way I can solve the above reimplement without shadowing a potentially still active bug.

samiebee43 added a commit that referenced this issue Sep 18, 2023
@samiebee43
Reference spec.fwhm instead of using default value 3
3ac241e 
Close #133
@samiebee43 samiebee43 mentioned this issue Sep 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1 - broken a userflow is impossible because something is broken prioritize There are reasons to do this sooner rather than later
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reference spec.fwhm instead of using default value 3 WasatchPhotonics/ENLIGHTEN
2 participants
@mzieg @samiebee43