pypeit / PypeIt

The Python Spectroscopic Data Reduction Pipeline
BSD 3-Clause "New" or "Revised" License
163 stars 104 forks source link

Combine refactor #1813

Closed jhennawi closed 4 months ago

jhennawi commented 5 months ago

This short PR refactors the CombineImage class to work on lists of images rather than lists of files. The file processing is moved into the calling routine pypeit.images.buildimage.py which makes more sense since this already takes the file list as the argument.

The advantages of this scheme is that it allows the class to work with JWST images for which the raw image reading routines do not work, because processing needs to be done to convert JWST outputs into PypeIt inputs. Furthermore, it produces a cleaner interface whereby CombineImage simply combines images but does not do file reading or image processing.

@rcooke-ast can you please see my comments about the arc lamp checking that I believe you added to this routine on line 291 of combineimage.py? My feeling is that checking specific things about the inputs to an image combining class should be performed outside of that class such that you pass in only the right images. The point is that others might want to check other things, and at some point the number of things we would check in this single purpose code would balloon. I think lamp exposure time checking should be done in calibrations.

codecov-commenter commented 5 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 73.23944% with 19 lines in your changes missing coverage. Please review.

Project coverage is 38.49%. Comparing base (9c7bb98) to head (66d5784).

Files Patch % Lines
pypeit/calibrations.py 43.33% 17 Missing :warning:
pypeit/images/combineimage.py 94.11% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1813 +/- ## =========================================== + Coverage 38.48% 38.49% +0.01% =========================================== Files 207 207 Lines 48153 48171 +18 =========================================== + Hits 18533 18545 +12 - Misses 29620 29626 +6 ```

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

kbwestfall commented 4 months ago

Lots of test failures:

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  256 passed, 6736 warnings in 508.76s (0:08:28) ---
--- PYTEST UNIT TESTS PASSED  148 passed, 2662 warnings in 916.21s (0:15:16) ---
--- PYTEST VET TESTS FAILED  9 failed, 52 passed, 92604 warnings in 697.22s (0:11:37) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 13/233 TESTS  ---
Failed tests:
    keck_deimos/600ZD_M_6500 pypeit
    keck_deimos/600ZD_tilted pypeit
    keck_deimos/1200B_M_5200 pypeit
    keck_deimos/1200B_LVM_5200 pypeit
    keck_deimos/900ZD_LVM_5500 pypeit
    keck_deimos/1200G_M_5500 pypeit
    mdm_osmos/MDM4K pypeit
    keck_mosfire/long2pos1_H pypeit
    shane_kast_red/300_7500_Ne pypeit
    bok_bc/600 pypeit
    keck_lris_red/long_150_7500_d560 pypeit
    keck_kcwi/small_bh2_4200 pypeit
    mdm_modspec/Echelle pypeit
Skipped tests:
    keck_deimos/600ZD_M_6500 pypeit_flux
    keck_deimos/600ZD_M_6500 pypeit_ql
    keck_deimos/600ZD_M_6500 pypeit_ql
    keck_deimos/900ZD_LVM_5500 pypeit_sensfunc
    keck_deimos/900ZD_LVM_5500 pypeit_flux
    keck_mosfire/long2pos1_H pypeit_coadd_2dspec
Testing Started at 2024-07-17T16:02:18.335840
Testing Completed at 2024-07-18T08:39:00.901060
Total Time: 16:36:42.565220

Except for the known bok_bc/600 failure, I think these all have to do with this line because self.files doesn't exist anymore. Given that #1822 changes this block of code, I'm just rerunning the tests using that branch instead. If they pass, we should just merge #1822 into this branch, consolidating the PRs.

rcooke-ast commented 4 months ago

Except for the known bok_bc/600 failure, I think these all have to do with this line because self.files doesn't exist anymore. Given that #1822 changes this block of code, I'm just rerunning the tests using that branch instead. If they pass, we should just merge #1822 into this branch, consolidating the PRs.

That's fine with me, but note that @jhennawi hasn't responded to my comment on PR #1822, so I'm not sure if he approves of my proposed changes.

kbwestfall commented 4 months ago

Tests pass

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  256 passed, 6736 warnings in 514.92s (0:08:34) ---
--- PYTEST UNIT TESTS PASSED  148 passed, 2662 warnings in 925.14s (0:15:25) ---
--- PYTEST VET TESTS PASSED  61 passed, 106001 warnings in 5542.49s (1:32:22) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 1/239 TESTS  ---
Failed tests:
    bok_bc/600 pypeit
Skipped tests:
Testing Started at 2024-07-19T15:34:33.772298
Testing Completed at 2024-07-20T10:06:24.521288
Total Time: 18:31:50.748990