Add application of DL1 charge calibration to CameraCalibrator #1160

watsonjj · 2019-10-28T14:37:23Z

This PR adds the ability to apply the DL1 charge calibration that has been attached to the event in the DL1CameraCalibrationContainer (event.mon.tel[telid].calibration.dl1)

ctapipe/calib/camera/calibrator.py

kosack · 2019-10-28T15:02:06Z

ctapipe/io/containers.py

+        "Coefficients for the relative correction between pixels to achieve a "
+        "uniform charge response (post absolute calibration) from a "
+        "uniform illumination."
+    )


For "relative" and "absolute", it may be better to name them "X_gain" or similar to be clear these are multiplicative calibration factors, unlike "pedestal" which is additive (well subtractive, really).

ctapipe/io/containers.py

watsonjj · 2019-10-28T15:11:19Z

I've added "_factor" to multiplicative coefficients, and "_offset" to additive coefficients

- Fix typo - Add test for coefficients default

codecov · 2019-10-28T17:04:30Z

Codecov Report

Merging #1160 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
+ Coverage    85.9%   85.97%   +0.07%     
==========================================
  Files         182      182              
  Lines       11305    11390      +85     
==========================================
+ Hits         9711     9792      +81     
- Misses       1594     1598       +4

Impacted Files	Coverage Δ
ctapipe/calib/camera/tests/test_calibrator.py	`100% <100%> (ø)`	⬆️
ctapipe/io/containers.py	`100% <100%> (ø)`	⬆️
ctapipe/calib/camera/calibrator.py	`96.19% <100%> (+0.15%)`	⬆️
ctapipe/io/simteleventsource.py	`97.6% <0%> (-0.92%)`	⬇️
ctapipe/io/hessioeventsource.py	`92.15% <0%> (-0.65%)`	⬇️
ctapipe/core/tests/test_traits.py	`100% <0%> (ø)`	⬆️
ctapipe/io/tests/test_simteleventsource.py	`100% <0%> (ø)`	⬆️
ctapipe/core/traits.py	`98.03% <0%> (+0.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cb7b96...ca2cd52. Read the comment docs.

kosack · 2019-10-29T13:29:08Z

what happens currently with monte-carlo data? Are these calibration monitoring containers filled with the values from the MC file?

Also, be clear that "monitoring" is not event-wise, it's time-wise, so if these are intended to be interpolated to an event time (in the case that things like the pedestals), then i'm not sure the monitoring value is the one you want to use.

watsonjj · 2019-10-29T13:35:00Z

what happens currently with monte-carlo data? Are these calibration monitoring containers filled with the values from the MC file?

No. The calibration coefficients depend on the charge extraction technique, and the calibration approach (e.g. converting to photons)

Also, be clear that "monitoring" is not event-wise, it's time-wise, so if these are intended to be interpolated to an event time (in the case that things like the pedestals), then i'm not sure the monitoring value is the one you want to use.

The values used here are intended to be the ones that you apply for an event. It may be that these are loaded once per observation, and don't change. Or it may be that they are interpolated from a table depending on the event time (and could be basically the same for multiple events). Or they may be continuously updated from an on-the-fly calibration. Let me know if this is not the correct place for such coefficients...

watsonjj · 2019-10-29T13:36:25Z

These values are intended to be filled by a Calculator class, which I will implement in a future PR (plus adapt Franca's Calculator classes)

kosack · 2019-10-30T10:25:27Z

The values used here are intended to be the ones that you apply for an event. It may be that these are loaded once per observation, and don't change. Or it may be that they are interpolated from a table depending on the event time (and could be basically the same for multiple events). Or they may be continuously updated from an on-the-fly calibration. Let me know if this is not the correct place for such coefficients...

If these are the instantaneous values for the event, not the "monitored values" once per second or more, then they really should be in the event, not monitoring container (I know we currently mix the two, but that will be separated at some point). Instantaneous values should be in the "DL1/Event/Telescope" data model (or R1 depending on when they are computed) and are derived generally from monitoring data. Perhaps for now, we just put them in event.calib.tel[x] to remind ourselves that they are the per-event quantities, so that when we remove all MON data from the event structure, we don't confuse the two?

Really, it's only pedestals I'm worried about confusing. Maybe also flatfield, but anything that doesn't change during an observation (DC/PE, etc) ,should eventually leave the event container completely.

kosack · 2019-10-30T10:30:56Z

I think I need to write down a complete proposal for the full internal and external data model at some point, but that will require some time (should be done and updated before a 1.0 release though!). For now, let's try to put things somewhere that makes it less painful to move them around later. Probably just not calling anything "Monitoring" now is best, since we so far do not really compute any true monitoring quantities (except perhaps the outputs from Franca's pedestal tool, if they are not per-event)

watsonjj · 2019-10-31T11:27:32Z

Really, it's only pedestals I'm worried about confusing. Maybe also flatfield, but anything that doesn't change during an observation (DC/PE, etc) ,should eventually leave the event container completely.

@kosack Does it make sense that it leaves the event container completely? It's important to know the correct value for the current event. As you have said in #1165:

Therefore what is in the event are instantaneous values of those quantities, interpolated to the current event time. Other COmponents that use them do not need to know how they were interpolated (or even if they were computed on-the-fly like with pedestals, perhaps), just that the value in the Container is the right one for this event. Therefore they don't need to distinguish between data coming from a "service" or from an event stream.

watsonjj · 2019-10-31T11:57:57Z

The dl1 calibration coefficients that are applicable for the current event (and should be used in the CameraCalibrator) are now located at event.calibration.tel[telid].dl1

kosack · 2019-10-31T15:26:31Z

@kosack Does it make sense that it leaves the event container completely? It's important to know the correct value for the current event. As you have said in #1165:

I think we keep the instantaneous values in the event. (just don't call them "mon")

watsonjj · 2019-10-31T17:21:46Z

I think we keep the instantaneous values in the event. (just don't call them "mon")

Yep done

watsonjj added 10 commits October 28, 2019 13:07

Replaced CameraCalibrationContainer with DL1CameraCalibrationContainer

da85ec1

Attach DL1CameraCalibrationContainer to MonitoringCameraContainer

61e59f4

Deprecate WaveformCalibrationContainer

afaccc4

Improve descriptions of calibration coefficients

2c1e755

Change default values

03f4da4

Apply calibration in CameraCalibrator

54c370f

Remove todo

e7eb597

Update description

6641de1

Add handling for when EventAndMonDataContainer is not used

a3321ab

Add test

44521f6

watsonjj added calibration ready for review labels Oct 28, 2019

maxnoe reviewed Oct 28, 2019

View reviewed changes

ctapipe/calib/camera/calibrator.py Outdated Show resolved Hide resolved

kosack requested changes Oct 28, 2019

View reviewed changes

More descriptive names for Multiplicative/Additive

6863d99

watsonjj added 3 commits October 28, 2019 16:33

Move DL1CameraCalibrationContainer into CameraCalibrationContainer

bada6d1

Rename time_offset to time_shift

28c01b3

- Fix typo - Add test for coefficients default

Correct __all__

367aa9e

kosack mentioned this pull request Oct 30, 2019

dc_to_pe should be pe_to_dc in SimTelEventSource and MCCameraEventContainer #1164

Open

Created event.calibration container

9a27d0e

Update docstrings

ca2cd52

kosack approved these changes Nov 5, 2019

View reviewed changes

watsonjj requested a review from maxnoe November 7, 2019 09:35

thomasarmstrong approved these changes Nov 12, 2019

View reviewed changes

watsonjj merged commit e0f58c8 into cta-observatory:master Nov 12, 2019

This was referenced Nov 13, 2019

Major refactoring of calibration chain #1026

Closed

Proposals for charge calibration flow #1147

Open

watsonjj mentioned this pull request Mar 7, 2020

Proposal: Refactor FlatFieldCalculator and PedestalCalculator #1226

Open

watsonjj deleted the dl1calib branch April 15, 2020 15:18

watsonjj mentioned this pull request Oct 14, 2020

Time flat fielding #1396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add application of DL1 charge calibration to CameraCalibrator #1160

Add application of DL1 charge calibration to CameraCalibrator #1160

watsonjj commented Oct 28, 2019 •

edited

Loading

kosack Oct 28, 2019

watsonjj commented Oct 28, 2019 •

edited

Loading

codecov bot commented Oct 28, 2019 •

edited

Loading

kosack commented Oct 29, 2019

watsonjj commented Oct 29, 2019

watsonjj commented Oct 29, 2019

kosack commented Oct 30, 2019 •

edited

Loading

kosack commented Oct 30, 2019

watsonjj commented Oct 31, 2019

watsonjj commented Oct 31, 2019

kosack commented Oct 31, 2019

watsonjj commented Oct 31, 2019

Add application of DL1 charge calibration to CameraCalibrator #1160

Add application of DL1 charge calibration to CameraCalibrator #1160

Conversation

watsonjj commented Oct 28, 2019 • edited Loading

kosack Oct 28, 2019

Choose a reason for hiding this comment

watsonjj commented Oct 28, 2019 • edited Loading

codecov bot commented Oct 28, 2019 • edited Loading

Codecov Report

kosack commented Oct 29, 2019

watsonjj commented Oct 29, 2019

watsonjj commented Oct 29, 2019

kosack commented Oct 30, 2019 • edited Loading

kosack commented Oct 30, 2019

watsonjj commented Oct 31, 2019

watsonjj commented Oct 31, 2019

kosack commented Oct 31, 2019

watsonjj commented Oct 31, 2019

watsonjj commented Oct 28, 2019 •

edited

Loading

watsonjj commented Oct 28, 2019 •

edited

Loading

codecov bot commented Oct 28, 2019 •

edited

Loading

kosack commented Oct 30, 2019 •

edited

Loading