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

fix for masked values in FitTrace #205

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Jan 11, 2024

This PR fixes some issues with FitTrace when bins are fully masked:

  1. If peak_method=gaussian, the fit will fail if even one column/bin is fully masked, with the error being raised by the fitter. This PR fixes that issue by setting the fit trace value to nan for that column, so it can proceed and fit the next bin. For the final fit to all bin peaks for the trace, these nans bins/columns will then be filtered.

  2. If peak_method=max or centroid, a warning when a fully masked bin is encountered claiming that it is 'Falling back on trace value from all-bin fit.'. This is not accurate to what is happening
    2a. for max, the argmax of that bin will be returned which is always 0 for a fully masked bin. To avoid skewing the final fit to all bins when 0, i think a more appropriate value for this is 'nan' which can then be filtered from the final fit. theres nothing to really 'fall back' to in this case. This will require fixing almost all of the tests, so for now in this PR i just modify the warning message and retain the current behavior. See ISSUE.
    2b. for centroid, when there is a fully masked bin it is also not 'falling back to the all bin fit'. to find the centroid of a fully masked bin, it attempts to interpolate between nans and will always return the maximum index in the bin. Like max, i am not changing the current behavior in this PR, that can be done as a follow up to resolve the corresponding ISSUE, but I change the warning message to reflect what is actually happening at the moment.

I also reorganized some stuff out of post_init into its own method, but this change is inconsequential.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b0b1b08) 81.61% compared to head (b8c65c3) 81.86%.

❗ Current head b8c65c3 differs from pull request most recent head cf68b9f. Consider uploading reports for the commit cf68b9f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
+ Coverage   81.61%   81.86%   +0.24%     
==========================================
  Files          10       10              
  Lines        1001     1009       +8     
==========================================
+ Hits          817      826       +9     
+ Misses        184      183       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

specreduce/tracing.py Outdated Show resolved Hide resolved
specreduce/tracing.py Outdated Show resolved Hide resolved
@cshanahan1 cshanahan1 marked this pull request as ready for review January 24, 2024 15:31
Copy link
Contributor

@tepickering tepickering 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. looks good!

@tepickering tepickering merged commit 2d49215 into astropy:main Jan 25, 2024
10 checks passed
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