Photon counting step function #260

kjl0025 · 2024-11-23T03:02:29Z

Describe your changes

Added a step function for photon counting frames.

Type of change

New feature (non-breaking change which adds functionality)

Reference any relevant issues (don't forget the #)

#2

Checklist before requesting a review

[X ] I have linted my code
[X ] I have verified that all unit tests pass in a clean environment and added new unit tests, as needed
[ X] I have verified that all docstrings are properly formatted and added new documentation, as needed

… fixed a minor error in combine.py almost finished with unit tests for photon counting

so that I can leave the "OPTIONAL" out of the recipe in prescan subtraction since we do want the calibrated bias offset ideally

…tests when it runs

kjl0025 · 2024-11-24T03:46:36Z

Okay, all done with my little changes now.

semaphoreP

Nice work! I added a bunch of small comments throughout. The two bigger comments are:

Should we have a separate piece of code to produce a photon-counted dark which will save it and allow it to be tracked in the caldb? Also should we have a separate step function to do the dark subtraction in photon counting mode?
Is the big unit test you wrote really an e2e test? And we should make it as such, and write a smaller unit test?

semaphoreP · 2024-11-25T16:20:29Z

corgidrp/mocks.py

@@ -166,7 +167,7 @@ def create_synthesized_master_dark_calib(detector_areas):
    # image area, including "shielded" rows and cols:
    imrows, imcols, imr0c0 = imaging_area_geom('SCI', detector_areas)
    prerows, precols, prer0c0 = unpack_geom('SCI', 'prescan', detector_areas)
-
+    np.random.seed(4567)


generally, seeding should be done by the function that calls this function, not this function itself (by doing it here, it will always produce the same result and can't be overridden).

semaphoreP · 2024-11-25T16:21:40Z

corgidrp/mocks.py

+        ill_mean (float): mean electron count value simulated in the illuminated frames
+        dark_mean (float): mean electron count value simulated in the dark frames
+    '''
+    np.random.seed(1234)


same comment with seeding as above.

semaphoreP · 2024-11-25T16:33:26Z

corgidrp/photon_counting.py

+    datasets, vals = test_dataset[0].split_dataset(prihdr_keywords=['VISTYPE'])
+    if len(vals) != 2:
+        raise PhotonCountException('There should only be 2 datasets, and one should have \'VISTYPE\' = \'DARK\'.')
+    dark_dataset = datasets[vals.index('DARK')]


should we be processing and producing a photon-counted dark separate for this step function? and then we feed in the photon-counted dark as a calibration file? I think this might be cleaner, and let us save the photon counted dark as an output.

I thought the only reason we made the synthesized and traditional master darks into calibration products was because they were CTC deliverables, so I didn't make the photon-counted dark into a separate calibration product because I don't think it is. Is this accurate, @mygouf ?

The photon-counting step function is basically the equivalent of mean-combine for an analog stack of frames. The photon-counted dark resulting from the stack of input darks is observation-specific, but if for some reason we do want it, I could add a return for it or add an ImageHDU for it in the output dataset?

semaphoreP · 2024-11-25T16:38:41Z

corgidrp/photon_counting.py

+    for dataset in [ill_dataset, dark_dataset]:
+        # getting number of read noise standard deviations at which to set the
+        # photon-counting threshold
+        T_factor = detector_params.params['T_factor']


should we allow an argument in this function to allow the user to override this value, rather than needing to modify detector_params? This seems like a tunable parameter, rather than a property of the detector that we configured.

semaphoreP · 2024-11-25T16:47:08Z

corgidrp/photon_counting.py

+        thresh = T_factor*read_noise
+        if thresh < 0:
+            raise PhotonCountException('thresh must be nonnegative')
+        if not isinstance(niter, (int, np.integer)) or niter < 1:


should we put this check at the start of the function, before we do any serious processing? I usually prefer that for organization sake to put the input error checking in the beginning.

semaphoreP · 2024-11-25T16:52:03Z

corgidrp/photon_counting.py

+        if not isinstance(niter, (int, np.integer)) or niter < 1:
+            raise PhotonCountException('niter must be an integer greater than '
+                                        '0')
+        try: # if EM gain measured directly from frame TODO change hdr name if necessary


I prefer to use statements like if 'EMGAIN_M' in dataset.frames[0].ext_hdr:, as it's more direct and doesn't require try/else (which feels overkill here).

semaphoreP · 2024-11-25T16:53:51Z

corgidrp/photon_counting.py

+        if thresh >= em_gain:
+            warnings.warn('thresh should be less than em_gain for effective '
+            'photon counting')
+        if np.average(dataset.all_data) > 0.1:


should we use np.nanmean in case there are nan's from cosmic rays?

Shouldn't be any NaNs at this point since the bad pixel step function is applied after this step function, but I can change it since it wouldn't hurt anything.

semaphoreP · 2024-11-25T17:07:07Z

corgidrp/recipe_templates/l1_to_l2b_pc.json

+            "calibs" : {
+                "DetectorParams" : "AUTOMATIC"
+            }
+        },


According to the FDD, there should be a step where we also mark pixels as bad using the master bad pixel map. I don't think this is being accounted for elsewhere.

I saw that that step happens after the photon-counting step?

semaphoreP · 2024-11-25T17:11:50Z

corgidrp/photon_counting.py

+        up = mean_expected_up +  pc_variance
+        low = mean_expected_low -  pc_variance
+        errs.append(np.max([up - mean_expected, mean_expected - low], axis=0))
+        dq = (nframes == 0).astype(int) 


Is there a reason specifically to use only the 0/1 bits of the DQ map, and not use the higher value bits? I don't seem to understand why we are erasing the reasons for bad DQ pixels. Can we just do an AND or some other logic to keep the history of which pixels are bad for what reason?

Essentially a mean-combine is performed on the frames, so the particular DQ flags become irrelevant (same process that is done with mean_combine).

semaphoreP · 2024-11-25T18:10:30Z

tests/test_photon_counting.py

+
+detector_params = data.DetectorParams({})
+
+def test_expected_results():


should this be an e2e test? It seems like it is testing the full use case like an e2e test. Another way to phrase the question: is there a different e2e test you were thinking of writing?

A unit test would test just the inputs and outputs of the step function (rather than going through the whole walker infrastructure).

I can do that!

Kevin Ludwick added 7 commits November 20, 2024 23:26

photon counting, fixed an err estimation from converting with k gain,…

62a928b

… fixed a minor error in combine.py almost finished with unit tests for photon counting

unit tests done, a few other small improvements

f181cce

linting

7ef22a9

lint

01482b6

lint

0d9f6f8

eliminated potential division by 0

948ccd2

make a detectornoisemaps cal file for CI's sake

8cabe71

so that I can leave the "OPTIONAL" out of the recipe in prescan subtraction since we do want the calibrated bias offset ideally

kjl0025 requested review from semaphoreP and maxwellmb November 23, 2024 05:22

Kevin Ludwick added 4 commits November 23, 2024 10:33

had to add nonlin calib as well

87f6d95

oops, forgot the dummy dataset

569ae0c

fewer full frames simulated in hopes that the CI will not cancel the …

e995bf3

…tests when it runs

oops, forgot to actually change the number

3b1b969

semaphoreP reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Photon counting step function #260

Photon counting step function #260

kjl0025 commented Nov 23, 2024

kjl0025 commented Nov 24, 2024

semaphoreP left a comment

semaphoreP Nov 25, 2024

semaphoreP Nov 25, 2024

semaphoreP Nov 25, 2024

kjl0025 Nov 26, 2024

kjl0025 Nov 26, 2024

semaphoreP Nov 25, 2024

semaphoreP Nov 25, 2024

semaphoreP Nov 25, 2024

semaphoreP Nov 25, 2024

kjl0025 Nov 26, 2024

semaphoreP Nov 25, 2024

kjl0025 Nov 26, 2024

semaphoreP Nov 25, 2024

kjl0025 Nov 26, 2024

semaphoreP Nov 25, 2024

kjl0025 Nov 26, 2024


		detector_params = data.DetectorParams({})

		def test_expected_results():

Photon counting step function #260

Are you sure you want to change the base?

Photon counting step function #260

Conversation

kjl0025 commented Nov 23, 2024

Describe your changes

Type of change

Reference any relevant issues (don't forget the #)

Checklist before requesting a review

kjl0025 commented Nov 24, 2024

semaphoreP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment