-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fix fit_lines with single quantity as window value #1164
Conversation
Looks like the one test failure is a remote data timeout. @TurboSlayer would you check to see if this fixes your use case? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1164 +/- ##
=======================================
Coverage 86.88% 86.89%
=======================================
Files 63 63
Lines 4544 4547 +3
=======================================
+ Hits 3948 3951 +3
Misses 596 596 ☔ View full report in Codecov by Sentry. |
Thanks - I managed to work around it by setting manual bounds anyways. I trust from the screenshots that you've fixed the issue haha. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding test to fit in frequency space might be good, or a test where spectral_axis is wavelength but the window in frequency (is that even allowed) but you can also argue expanding test suite is out of scope here.
Needs a change log?
You're not my boss! (yes, yes, I'll add one momentarily) |
Indeed I am not. But I can unleash the change log bot here if you want. 😉 (#1165) |
Remote data failure unrelated and can be ignored. |
Fixes #1160. Currently on main, giving a single Quantity as input for
window
gets ignored, since a Quantity is neither anint
nor afloat
. You can see below that one of the fits in the fitting tests is actually very bad, since it's fitting the whole spectrum and doesn't limit to a window around the left gaussian:With this fix, the fit for the left gaussian in the test case properly gets limited to the defined window and is fit well: