-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Compatibility with specutils 2.0 #260
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
+ Coverage 87.19% 87.27% +0.08%
==========================================
Files 13 15 +2
Lines 1179 1289 +110
==========================================
+ Hits 1028 1125 +97
- Misses 151 164 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Hmm devdeps fail for me locally with astropy 7.0.1 but not here using astropy 7.1.dev. So this means something unreleased in astropy is required for specreduce to work with specutils 2.0, which probably means specutils 2.0 cannot be released before either astropy 7.0.2 or 7.1, depending on what PR is involved over at astropy; @rosteen do you remember?
@@ -303,12 +303,18 @@ def bkg_image(self, image=None): | |||
|
|||
Returns | |||
------- | |||
`~specutils.Spectrum1D` object with same shape as ``image``. | |||
spec : `~specutils.Spectrum1D` |
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.
The Sphinx ref will have to be updated separately after specutils 2.0 stable is released and RTD picks up specutils 2.0. Might have to do a special requirements file for doc build or pin specutils>=2
here:
Line 25 in 6a0b915
docs = [ |
But nothing we can do right now.
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.
ah, ignore my other comment, then.
|
||
>>> standard_sensfunc(obj_wave, obj_flux, stdstar='spec50cal/bd284211.dat', mode='spline') # doctest: +SKIP |
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.
If there is no intention to run this through doctest, omitting >>>
is enough to stop it from being collected by pytest-doctestplus.
and fix style check
specreduce/tests/test_background.py
Outdated
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.
There are 2 failures if you use astropy 7.0.1 instead of astropy 7.1.dev together with specutils 2.0rc1. See the failures at https://github.com/astropy/astropy-integration-testing/actions/runs/13934821756/job/39000997976?pr=26
based on all the work required here, i suspect this transition will be painful for a lot of packages that use |
Doh... The devdeps here is I see the same failures locally with specutils 2.0 + astropy 7.1.dev , so the compatibility problem remains. |
@@ -151,7 +151,7 @@ def test_fit_trace(): | |||
window = 10 | |||
guess = int(nrows / 2) | |||
img_win_nans = img.copy() | |||
img_win_nans[guess - window : guess + window] = np.nan | |||
img_win_nans[guess - window: guess + window] = np.nan |
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.
My local flake8 run was complaining about the whitespace but it is strictly unrelated. I can revert such changes if it bothers you.
Okay, mystery solved. devdeps here now gives the same traceback as integration testing when I switch dev specutils to install from its 2.0 branch. |
Fix #259
Plus some minor clean-ups.