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

Small tweaks to SDSS loaders #1217

Merged
merged 5 commits into from
Mar 6, 2025
Merged

Small tweaks to SDSS loaders #1217

merged 5 commits into from
Mar 6, 2025

Conversation

havok2063
Copy link
Contributor

This PR makes some small tweaks to the SDSS data loaders for SDSS-IV APOGEE data. The existing loaders were not correctly auto-identifying the spectrum format, for data from DR17 or from the southern hemisphere LCO 2.5-m. This fixes that.

Additionally it makes a small tweak to the SDSS-IV spec loaders for BOSS, compared to the SDSS-V loaders, to differentiate the two. Previously identify_spectrum_format would return a list ["SDSS-III/IV spec", "SDSS-V spec"] which breaks expectations for identify_spectrum_format. With the tweak, SDSS spec files from SDSS-IV now correctly return the single format "SDSS-III/IV spec", whereas SDSS-V spec data returns "SDSS-V spec".

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.89%. Comparing base (e761853) to head (618d1e3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1217   +/-   ##
=======================================
  Coverage   86.89%   86.89%           
=======================================
  Files          63       63           
  Lines        4572     4572           
=======================================
  Hits         3973     3973           
  Misses        599      599           

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@havok2063
Copy link
Contributor Author

@rosteen small ping on this, just thinking about specutils 2.0, or the next release of 1.x. Is it possible to get this in for then?

@rosteen
Copy link
Contributor

rosteen commented Mar 5, 2025

@havok2063 thanks for the ping, I can take a look tomorrow. I'm trying to get a 2.0 release candidate out in the next couple weeks and release by the end of March, is that timeline ok for this? I wasn't planning to do another 1.x but if it's more urgent I could.

@havok2063
Copy link
Contributor Author

Thanks @rosteen! It's not urgent. That timeline works just fine!

Copy link
Contributor

@rosteen rosteen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me.

@rosteen rosteen merged commit 583e136 into astropy:main Mar 6, 2025
13 checks passed
@pllim
Copy link
Member

pllim commented Mar 7, 2025

I think this broke Jdaviz test. Does this mean Jdaviz test had been wrong all along until now, or this patch is incomplete?

_______________ test_get_valid_format[SDSS-III/IV spec-specviz] ________________

create_fake_fits = <function create_fake_fits.<locals>._create_fits at 0x7f1597272d40>
name = 'SDSS-III/IV spec', expconf = 'specviz'

    @pytest.mark.parametrize('name, expconf',
                             [('MaNGA cube', 'cubeviz'),
                              ('MaNGA rss', 'specviz'),
                              ('JWST s3d', 'cubeviz'),
                              ('JWST s2d', 'specviz2d'),
                              ('JWST x1d', 'specviz'),
                              ('JWST c1d', 'specviz'),
                              ('JWST x1d multi', 'specviz'),
                              ('JWST c1d multi', 'specviz'),
                              ('SDSS-III/IV spec', 'specviz'),
                              ('generic 3d', 'cubeviz'),
                              ('generic 1d', 'specviz')])
    def test_get_valid_format(create_fake_fits, name, expconf):
        """ test correct file format and config is returned """
        filename = create_fake_fits(name)
    
        # for generic files that have no registered specutils format, valid_format will be blank
        name = name if 'generic' not in name else []
    
        valid_format, config = get_valid_format(filename)
>       assert (valid_format, config) == (name, expconf)
E       AssertionError: assert ([], 'specviz2d') == ('SDSS-III/IV...c', 'specviz')
E         
E         At index 0 diff: [] != 'SDSS-III/IV spec'
E         
E         Full diff:
E           (
E         -     'SDSS-III/IV spec',
E         +     [],
E         -     'specviz',
E         +     'specviz2d',
E         ?             ++
E           )

jdaviz/tests/test_data_formats.py:119: AssertionError

@havok2063
Copy link
Contributor Author

@pllim The test expectation ('SDSS-III/IV spec', 'specviz') is correct. Something in create_fake_fits or get_valid_format might need updating. I'll take a look.

@pllim
Copy link
Member

pllim commented Mar 7, 2025

Thanks for the response! @rosteen has a downstream patch at spacetelescope/jdaviz#3489

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