Fix for Astropy v7.0 incompatibility (#229)

[hpparvi]

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

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.33%. Comparing base (ef38dc0) to head (c053b33).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
specreduce/tracing.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
- Coverage   83.36%   83.33%   -0.03%     
==========================================
  Files          13       13              
  Lines        1136     1140       +4     
==========================================
+ Hits          947      950       +3     
- Misses        189      190       +1     

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

[pllim]

Please rebase to pick up the other CI fix.

[pllim]

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).

[hpparvi]

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]

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

[hpparvi]

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]

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]

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

[hpparvi]

Any opinions, @cshanahan1, @kecnry, @tepickering? I think we can adopt DogBoxLSQFitter without any problems.

[pllim]

Also astropy 7.0 is already released, so this is a 🔥 now. If you don't hear back, I'd say you make a decision in your role as project manager and ping me to merge since I don't think you can merge yet?

[tepickering]

agreed this is top priority to fix ASAP. i'd go ahead and make the switch to DogBoxLSQFitter.

[hpparvi]

agreed this is top priority to fix ASAP. i'd go ahead and make the switch to DogBoxLSQFitter.

Thanks @tepickering! I've changed the fitting method and also added a check to use LinearLSQFitter for linear trace models like Chebyshev1D.

@pllim, this PR should be ready to merge now. I haven't been granted a maintainer status quite yet, but the two-week comment period is over now without any opposing comments (astropy/astropy.github.com#639), so this should change soon.

[pllim]

Thanks!

[pllim]

Does this need a change log?

[tepickering]

thanks for catching all this! i like adding the checks to help choose the optimal fitter.

[tepickering]

Does this need a change log?

i think so since it's a pretty significant change to how model fitting is done. once that's added, i can go ahead and merge.

[hpparvi]

Does this need a change log?

Good point! I've added the changes to the change log under v1.4.2.

[pllim]

Since Tim said he would merge, I'll give him the honors. But if he is MIA, please ping me again because I can also merge but would rather let package maintainers do this. Thanks, all!