Combine refactor #1813

jhennawi · 2024-06-03T17:11:36Z

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.

kbwestfall

Looks good to me. I think the only requests I have are minor doc updates. But we should talk more about moving the lamp checking code and whether we want to make that change in this PR.

pypeit/images/combineimage.py

kbwestfall · 2024-06-03T18:31:52Z

pypeit/images/combineimage.py


+        # TODO JFH: This should not be here, but rather should be done in the Calibrations class. Putting in calibration specific
+        # lamp checking in an image combining class is not ideal. 


In principle, I agree. A lamp check that is specific to arcs is out of place in a generic image-combining class. We could push the lamp check into Calibrations.get_arc, but we need to be careful about the amount of code that would need to be repeated and minimize the number of times we need to open the raw files.

I would say that the exposure time check should stay here (and we should deal with combining images with different exposure times better...).

I agree. In fact, since we are now passing in a pypeitImage (and not doing the reading/processing in this function as before), we should move the lamp checks to pypeit/images/buildimage.buildimage_fromlist(). This allows @jhennawi to remove the fudged function in jwst_nirspec() I think this makes the most sense.

I mostly agree with @kbwestfall that the exposure time stuff should stay here though, although I could be convinced either way, and that might be worth a separate discussion.

@rcooke-ast this arc lamp checking stuff was not added by me in this PR, but rather I think by you in a different context. Can you please confirm? All I did was move that bit of code here from another of the image combine routines in order to be compatible with the refactor.

It's a simple change... I will submit a small PR into this so you can see what I mean. I just didn't want to monkey around with your PR.

OK @jhennawi - see PR #1822. I haven't tested this with any JWST data, so you might want to check it solves the issue you were having.

pypeit/images/combineimage.py

pypeit/images/rawimage.py

kbwestfall · 2024-06-03T18:57:12Z

pypeit/pypeit.py

@@ -789,6 +789,9 @@ def objfind_one(self, frames, det, bg_frames=None, std_outfile=None):
            bkg_redux_sciimg = sciImg
            # Build the background image
            bg_file_list = self.fitstbl.frame_paths(bg_frames)
+            # TODO I think we should create a separate self.par['bkgframe'] parameter set to hold the image
+            # processing parameters for the background frames.  This would allow the user to specify different
+            # parameters for the background frames than for the science frames.  


Let's talk more about this.

codecov-commenter · 2024-06-03T23:17:35Z

⚠️ Please install the 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 ⚠️
pypeit/images/combineimage.py	94.11%	2 Missing ⚠️

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

Additional details and impacted files

@@             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

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

debora-pe

It look good to me. But we need to run the tests!

rcooke-ast

The PR looks good to me. I only have one request (+ running the tests as usual)

kbwestfall · 2024-07-18T15:07:39Z

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 · 2024-07-18T15:13:15Z

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.

Move lampstat check

kbwestfall · 2024-07-22T15:09:38Z

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

rcooke-ast

Looks good - thanks!

rcooke-ast · 2024-06-29T12:33:50Z

pypeit/images/combineimage.py


+        # TODO JFH: This should not be here, but rather should be done in the Calibrations class. Putting in calibration specific
+        # lamp checking in an image combining class is not ideal. 


OK @jhennawi - see PR #1822. I haven't tested this with any JWST data, so you might want to check it solves the issue you were having.

jhennawi added 5 commits June 1, 2024 17:34

refactoring pypeit image

5626861

new

383ac48

refactored image combining to work on a set of image objects

db76343

removed unncessary parameter arguments

85917f7

fixed some issues with saturation masking

2463c70

jhennawi requested review from profxj, rcooke-ast and kbwestfall June 3, 2024 17:11

kbwestfall approved these changes Jun 3, 2024

View reviewed changes

Merge branch 'develop' into combine_refactor

9d66d73

test fix

eba291f

debora-pe approved these changes Jun 4, 2024

View reviewed changes

rcooke-ast requested changes Jun 5, 2024

View reviewed changes

jhennawi and others added 6 commits June 9, 2024 13:45

some updates

68aad10

Merge branch 'develop' into combine_refactor

634f27c

Move lampstat check

1deaeb3

Move lampstat check to calibrations

69700a3

Merge branch 'develop' into combine_refactor

1827eb3

Merge branch 'combine_refactor' into combine_refactor_delta

e509422

jhennawi and others added 6 commits July 19, 2024 21:27

Updated docs on check_calibraitons

62ddd8a

Tuned up buildimage to just loop over the file.

0373f38

Merge pull request #1822 from pypeit/combine_refactor_delta

fe00d01

Move lampstat check

addressing PR comments.

58391cd

addressing PR comments.

13f0dc4

adressing pr comments

461260f

Merge branch 'develop' into combine_refactor

66d5784

rcooke-ast approved these changes Jul 22, 2024

View reviewed changes

kbwestfall merged commit 9e19d4c into develop Jul 22, 2024
23 checks passed

kbwestfall deleted the combine_refactor branch July 22, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine refactor #1813

Combine refactor #1813

jhennawi commented Jun 3, 2024

kbwestfall left a comment

kbwestfall Jun 3, 2024

rcooke-ast Jun 5, 2024

jhennawi Jun 29, 2024

rcooke-ast Jun 29, 2024

rcooke-ast Jun 29, 2024

kbwestfall Jun 3, 2024

codecov-commenter commented Jun 3, 2024 •

edited

Loading

debora-pe left a comment

rcooke-ast left a comment

kbwestfall commented Jul 18, 2024

rcooke-ast commented Jul 18, 2024

kbwestfall commented Jul 22, 2024

rcooke-ast left a comment

rcooke-ast Jun 29, 2024


		# TODO JFH: This should not be here, but rather should be done in the Calibrations class. Putting in calibration specific
		# lamp checking in an image combining class is not ideal.

Combine refactor #1813

Combine refactor #1813

Conversation

jhennawi commented Jun 3, 2024

kbwestfall left a comment

Choose a reason for hiding this comment

kbwestfall Jun 3, 2024

Choose a reason for hiding this comment

rcooke-ast Jun 5, 2024

Choose a reason for hiding this comment

jhennawi Jun 29, 2024

Choose a reason for hiding this comment

rcooke-ast Jun 29, 2024

Choose a reason for hiding this comment

rcooke-ast Jun 29, 2024

Choose a reason for hiding this comment

kbwestfall Jun 3, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jun 3, 2024 • edited Loading

Codecov Report

debora-pe left a comment

Choose a reason for hiding this comment

rcooke-ast left a comment

Choose a reason for hiding this comment

kbwestfall commented Jul 18, 2024

rcooke-ast commented Jul 18, 2024

kbwestfall commented Jul 22, 2024

rcooke-ast left a comment

Choose a reason for hiding this comment

rcooke-ast Jun 29, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jun 3, 2024 •

edited

Loading