astropy / specutils

An Astropy coordinated package for astronomical spectroscopy. Maintainers: @nmearl @rosteen @keflavich @eteq
http://specutils.readthedocs.io/en/latest/
161 stars 124 forks source link

Add a DESI file reader #1116

Closed weaverba137 closed 5 months ago

weaverba137 commented 7 months ago

This PR closes #1103.

I'd like to add more tests before merging, but people can start taking a look at this.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (fb5bd08) 70.87% compared to head (251cd61) 70.76%. Report is 1 commits behind head on main.

:exclamation: Current head 251cd61 differs from pull request most recent head 15fdad2. Consider uploading reports for the commit 15fdad2 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1116 +/- ## ========================================== - Coverage 70.87% 70.76% -0.12% ========================================== Files 64 61 -3 Lines 4498 4210 -288 ========================================== - Hits 3188 2979 -209 + Misses 1310 1231 -79 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

weaverba137 commented 6 months ago

@eteq, @rosteen, @nmearl, @keflavich, I'm looking for advice on adding additional tests to this PR. Specifically:

  1. Are there examples of how to test file identifiers, in addition to file readers?
  2. Is the existing test configuration for the DESI reader sufficient, even though it will only ever be activated with e.g. --remote-data, or would you prefer smaller versions of the DESI data files to be added to the package itself?

Also, not a question but a comment: The code coverage configuration in setup.cfg, specutils/*/tests/*, does not exclude specutils/io/asdf/tests or specutils/io/default_loaders/tests, so I added specutils/*/*/tests/*.

weaverba137 commented 6 months ago

@eteq, @rosteen, @nmearl, @keflavich, post-holiday ping.

rosteen commented 5 months ago

@eteq, @rosteen, @nmearl, @keflavich, post-holiday ping.

@weaverba137 thanks for the ping, I'll try to respond to your questions later today.

rosteen commented 5 months ago
1. Are there examples of how to test file identifiers, in addition to file readers?

I think the best way to do this is by simply calling Spectrum1D.read() or SpectrumList.read() (whichever is appropriate) rather than calling coadd_loader and spectra_loader directly in your tests. This way the tests will first go through the identifier and then get routed to the relevant loader.

2. Is the existing test configuration for the DESI _reader_ sufficient, even though it will only ever be activated with _e.g._ `--remote-data`, or would you prefer smaller versions of the DESI data files to be added to the package itself?

Most of the existing tests, if I'm remembering correctly, create a fake FITS file on the fly programmatically that mimic the structure of the relevant files - see for example the JWST reader tests. It seems like the DESI files are pretty large, so I would say that either that strategy or creating a minimal DESI file (by, e.g., opening a real file with astropy.io.fits and deleting/shrinking the data arrays) to include in the repo (or host somewhere and have it still be remote data) would be preferable.

weaverba137 commented 5 months ago

@rosteen, thank you, those are good suggestions.

weaverba137 commented 5 months ago

I think this is now ready for final review.

weaverba137 commented 5 months ago

@rosteen, thank you, those are all reasonable suggestions, I'll get those in soon.

weaverba137 commented 5 months ago

All requested changes are in. Tests are failing due to a temporary failure in name resolution:

astropy.utils.iers.iers.IERSWarning: failed to download https://datacenter.iers.org/data/9/finals2000A.all: <urlopen error [Errno -3] Temporary failure in name resolution>

which is unrelated to this PR.