specutils icon indicating copy to clipboard operation
specutils copied to clipboard

Minor changes to DC loaders for future surveys

Open aragilar opened this issue 1 year ago • 2 comments

This makes it easier to use a fallback header and access the last spectra from a wrapper loader. These changes are to support loading some of the IFS surveys (such as SAMI) that Data Central hosts.

aragilar avatar Feb 06 '24 05:02 aragilar

Codecov Report

Attention: Patch coverage is 91.42857% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 86.77%. Comparing base (5ec4931) to head (ef70cdb). Report is 3 commits behind head on main.

Files Patch % Lines
specutils/io/default_loaders/dc_common.py 42.85% 4 Missing :warning:
specutils/io/default_loaders/sami.py 96.82% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1122       +/-   ##
===========================================
+ Coverage   72.22%   86.77%   +14.55%     
===========================================
  Files          62       63        +1     
  Lines        4432     4500       +68     
===========================================
+ Hits         3201     3905      +704     
+ Misses       1231      595      -636     

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

codecov[bot] avatar Feb 06 '24 05:02 codecov[bot]

Sorry, this dropped down my todo list, as a test of the changes, I've added the first pass of the SAMI loaders we're working on to the PR, which use the changed functionality.

aragilar avatar Mar 14 '24 04:03 aragilar

Sorry, this dropped down my todo list, as a test of the changes, I've added the first pass of the SAMI loaders we're working on to the PR, which use the changed functionality.

Looking at the code coverage, it seems like the SAMI loaders are never getting called during testing, so I suspect that the identifier for the new loader isn't positively identifying the files you uploaded.

rosteen avatar Apr 16 '24 14:04 rosteen

I've modified the tests slightly, that should directly flag whether the SAMI loaders are being used or not (which is weird because it works locally).

aragilar avatar Apr 19 '24 08:04 aragilar

Oh, it looks like the tests are being skipped, if you look at test_loaders in the coverage run, there's a whole bunch of tests marked as skip. I guess they should be not skipped, which will likely improve coverage all round.

aragilar avatar Apr 19 '24 08:04 aragilar

I've rebased this on top of #1132 to see if this actually shows the right coverage now.

aragilar avatar Apr 26 '24 06:04 aragilar

The SAMI file coverage is as expected now.

aragilar avatar Apr 26 '24 06:04 aragilar

LGTM now that coverage is reporting properly.

rosteen avatar Apr 30 '24 14:04 rosteen