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 Astropy v7.0 incompatibility (#229) #232

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hpparvi
Copy link
Contributor

@hpparvi hpparvi commented Nov 15, 2024

This PR fixes Issue #229 by increasing the absolute and relative tolerances of assert_allclose in test_fit_trace_all_nan_col. An absolute tolerance of 0.05 pixels should be sufficient to test that the tracing methods work correctly but leave enough room for any minor variations in the results caused by the changes in the LevMarLSQFitter between Astropy v6 and v7.

Fix #229

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.36%. Comparing base (ef38dc0) to head (8b0cd21).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage   83.36%   83.36%           
=======================================
  Files          13       13           
  Lines        1136     1136           
=======================================
  Hits          947      947           
  Misses        189      189           

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

@pllim
Copy link
Member

pllim commented Nov 15, 2024

Please rebase to pick up the other CI fix.

@pllim
Copy link
Member

pllim commented Nov 15, 2024

The broader question, though, is whether specreduce should still be using a fitter that is no longer recommended by core astropy library (astropy/astropy#16983).

…e' tests again. They don't matter in practise when we set the absolute tolerances to 0.05 pix.
@hpparvi
Copy link
Contributor Author

hpparvi commented Nov 15, 2024

The broader question, though, is whether specreduce should still be using a fitter that is no longer recommended by core astropy library (astropy/astropy#16983).

It probably shouldn't. Fixing this should be relatively trivial, and I can do it either in this PR or a separate one.

@pllim
Copy link
Member

pllim commented Nov 15, 2024

Actually I am wondering also if relaxing tolerance is even needed if you switch fitter class.

…ropy.modeling.fitting.LevMarLSQFitter` in `specreduce.tracing.FitTrace`.
@hpparvi
Copy link
Contributor Author

hpparvi commented Nov 15, 2024

It seems still to be required. I've changed the fitter and run the tests, and the results still differ slightly (at a 0.01-pixel level).

@hpparvi
Copy link
Contributor Author

hpparvi commented Nov 15, 2024

The change to LMLSQFitter leads to a deprecation warning: "Using LMLSQFitter for models with bounds is now deprecated since astropy 7.0", so I've also given the TRFLSQFitter and DogBoxLSQFitter a try. The latter works quite well in the tests, but the former results in some larger discrepancies (>0.3 pix). I can either change the test to ignore the deprecation warning, or change the code to use DogBoxLSQFitter with slightly increased error tolerances.

Any preferences? Considering the future, I'd prefer to go with DogBoxLSQFitter.

@pllim
Copy link
Member

pllim commented Nov 15, 2024

I'll defer to specreduce maintainers on which fitters to replace the old one with. Thanks!

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.

astropy v7.0 incompatibility in TestMasksTracing.test_fit_trace_all_nan_cols
2 participants