First step of implementing CEP-002: Change container structure #2204

maxnoe · 2023-01-10T16:10:21Z

No description provided.

maxnoe · 2023-01-10T17:54:56Z

Before I continue changing everything, could you review the new container structure?

I think it's already a big step in the right direction, as the structure is much simpler, we could get rid of many indirections...

nbiederbeck · 2023-01-11T09:17:22Z

~~This comment just looked at ctapipe/containers.py.~~ In general I think the structure looks fine, it makes more sense as seen in the tests, ~~but while we're at it,~~ some comments:

For the EventContainers: and why do we have count?
Should there be a general MuonContainer?
Why does the TelescopePointingContainer have no ra/dec?
Or why does ArrayPointingContainer have it?
Should we rename the ParticleClassificationContainer and TelescopeImpactParameterContainer to be more consistent with ReconstructedEnergyContainer and ReconstructedGeometryContainer? E.g. ReconstructedParticleContainer, ReconstructedImpactContainer.
Is Reconstructed[…].telescopes sensible to be None if Mono? Or should it just hold the corresponding tel_id and be of type int?
Is TelescopeTriggerContainer.n_trigger_pixels with default -1 sensible? Or is 0 better to have an unsigned int?
SimulationConfigContainer have many nans, but str seems more appropriate given the description. Or None.
TelescopeSimulationContainer.impact is not called true_impact.
MorphologyContainer, is -1 a better default than 0? Does it make a difference of default 0 and filled 0?
Is CameraTimingParametersContainer still needed?
LeakageContainer.intensity_width_{1,2} is ambiguously documented (intensity vs. fraction of intensity, as of the docstring).
There are some Camera-containers, which I think we can get rid of?

maxnoe · 2023-06-05T08:32:00Z

Why does the TelescopePointingContainer have no ra/dec?

Because all our telescopes are alt/az mounts, so this is the actual information that is monitored.

Or why does ArrayPointingContainer have it?

Because that's the reference point the array is tracking, which might be in sky-fixed coordinates.

TelescopeSimulationContainer.impact is not called true_impact.

Isn't this accomplished by adding the true_ prefix in the data writer?

Is TelescopeTriggerContainer.n_trigger_pixels with default -1 sensible? Or is 0 better to have an unsigned int?

I think it's important to distinguish invalid from valid data, 0 trigger pixels are possible (e.g. for pedestal events which are randomly triggered), so better distinguish. Same for the other positive integers.

maxnoe · 2023-06-05T08:33:23Z

For the EventContainers: and why do we have count?

Just nice to have I guess.

maxnoe · 2023-06-05T08:35:26Z

Is CameraTimingParametersContainer still needed?

That's connected to all the other Camera... containers and to #2141, we need them as long as we still support computing image parameters in CameraFrame

StFroese · 2023-07-07T09:38:13Z

The ArrayEventContainer contains the DL2 data (geometry, energy, classification) of the event in the ArrayDL2Container.
It also contains the telescopes as a map of TelescopeEventContainer's which contains all data levels like r0... and also dl2. dl2 in this case is a TelescopeDL2Container which is a subclass of the ArrayDL2Container again.

Why do we save the array event reconstructions at telescope level again? This is a little bit confusing to me

maxnoe · 2023-07-07T09:51:54Z

Why do we save the array event reconstructions at telescope level again? This is a little bit confusing to me

The DL2TelescopeContainer is for monoscopic predictions, not for repeating the subarray info.

The inheritance here is just to get the same fields, but I agree that it's confusing that isinstance(DL2TelescopeContainer, DL2SubarrayContainer). I had an intermediate version where both just inherited from Container and then duplicated the field definitions.

review-notebook-app · 2023-07-10T15:11:21Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

kosack · 2023-07-07T14:40:04Z

ctapipe/image/image_processor.py

@@ -209,51 +210,49 @@ def _parameterize_image(
        # parameterization
        return default

-    def _process_telescope_event(self, event):
+    def _process_telescope_event(self, tel_event):


I'd suggest adding type hints for functions like this while you are at it, especially for ones where the type changed.

def _process_telescope_event(self, tel_event: DL1TelescopeContainer):

That will make it more clear what is expected where, and also support language servers and mypy to do better completion, etc.

It's a TelescopeEventContainer, but yes, I'll add type hints for these. However, these are not terribly helpful, since at least my lsp only looks at the class variable, not the instance variables or rather assumes they are of the same type.

StFroese · 2023-07-10T22:50:48Z

The inheritance here is just to get the same fields, but I agree that it's confusing that isinstance(DL2TelescopeContainer, DL2SubarrayContainer). I had an intermediate version where both just inherited from Container and then duplicated the field definitions.

Can we add a ReconstrucedShowerContainer which contains the fields and DL2{Telescope|Subarray}Container inherit from that?

maxnoe · 2023-10-31T17:01:37Z

Hi all,

I rebased this on the current main and squashed the commits into one, to ease follow-up rebases when we make updates to main.

I'd appreciate a fresh review now.

I suggest to first only review the container structure in ctapipe.containers (beware, this is collapsed by default) before reviewing any of the resulting code changes.

Once we are happy with the container structure and renaming, I'll fix up the rest of the code and we can commence the review there.

maxnoe · 2023-10-31T17:04:38Z

ctapipe/containers.py

@@ -759,12 +748,11 @@ class SimulatedCameraContainer(Container):
    )


-class SimulatedEventContainer(Container):
+class SimulationSubarrayContainer(Container):
    shower = Field(


Right now, simulationsubarrayContainer has a single subcontainer, shower.

We could just attach the shower container at the SubarrayEventContainer to get rid of one level of indirection.

ccossou · 2023-11-06T10:16:59Z

For the EventContainers: and why do we have count?

Just nice to have I guess.

On this part. This, to me, is related to how people parse the file and shouldn't be in the container itself, or if it is, should be hidden and not publicly exposed (even if in python, private just means harder to type). But that's a minor comment and not the main purpose of CEP-002.

ccossou · 2023-11-07T14:02:15Z

ctapipe/image/image_processor.py

            )
+            for container in simulation.true_parameters.values():


If the prefix doesn't start with "true_", doesn't that mean there's something wrong with the file and we should raise an error rather than silently correcting it?

If the prefix doesn't start with "true_", doesn't that mean there's something wrong with the file

Has nothing to do with the input file, since the container is created by the _parametrize_image methods, but you are right, this is confusing.

I will check if it is needed / can be improved.

ctapipe/image/image_processor.py

ccossou · 2023-11-07T14:10:31Z

(Sorry, commented on file without realizing it wasn't container.py anymore, got carried away.)

maxnoe · 2024-04-30T13:23:15Z

Wow, resolving the conflicts here was no fun.... I'd really appreciate if we can tackle this one next.

I will do a self-review again and it would be nice if others could start reviewing the structure in src/ctapipe/containers.py

One particular issue (noted with a fixme in the CameraCalibrator) is that we (in accordance with the CTAO data model) moved the trigger information to dl0, but the SimTelEventSource and also other event source implementations are allowed to return event data with only R1 inside.

This has the issue of the CameraCalibrator.r1_to_dl0 method needing to fill trigger information... which it now does by checking if there is a pre-existing dl0 container. Not so nice conceptually.

I see two solutions:

Putting trigger info also at R1 so that e.g. CameraCalibrator can just forward it from r1 to dl0
Requiring event sources to provide dl0, removing the dvr step from CameraCalibrator so that it only deals with the calibration from dl0 to dl1

I think the latter is the option that is more closely aligned with how we expect actual CTAO operations to be.

Studies for DVR can still be done, if we for example at dvr options to simteleventsource or in an intermediate step.

maxnoe · 2024-04-30T13:37:45Z

src/ctapipe/containers.py

+        description="DL2 reconstructed event properties",
+    )
+
+    pointing = Field(


Move into dl0?

Not sure, is it very helpful?

Putting pointing in dl0 means you can't make geometry reconstruction using pure dl2 files, but maybe that's fine? Not sure what other costs there are.

DL2 files could always include the dl0 monitoring table. Independent of where this field is stored in the object, it is written to /dl0 in the hdf5 data files.

There is also pointing information in the tel field below, although it is per telescope rather than for the subarray.

Putting the subarray pointing info into the dl0 now seems strange, are there any quantities that depend on the subarray pointing at dl0? Isn't the telescope pointing the natural pointing until you do stereo?

src/ctapipe/calib/camera/tests/test_calibrator.py

src/ctapipe/io/tableloader.py

src/ctapipe/image/reducer.py

src/ctapipe/instrument/trigger.py

Tobychev · 2024-05-13T12:32:52Z

src/ctapipe/containers.py

+        description="DL2 reconstructed event properties",
+    )
+
+    pointing = Field(


Not sure, is it very helpful?

Putting pointing in dl0 means you can't make geometry reconstruction using pure dl2 files, but maybe that's fine? Not sure what other costs there are.

src/ctapipe/io/datawriter.py

src/ctapipe/io/hdf5eventsource.py

kosack

I don't see any major problem.

ctao-dpps-sonarqube · 2024-07-15T13:23:36Z

Analysis Details

12 Issues

0 Bugs
0 Vulnerabilities
12 Code Smells

Coverage and Duplications

96.30% Coverage (93.70% Estimated after merge)
1.09% Duplicated Code (0.60% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

Tobychev · 2024-12-05T23:05:36Z

src/ctapipe/containers.py

@@ -262,6 +272,25 @@ class TelEventIndexContainer(Container):
    tel_id = tel_id_field()


+class TelescopeTriggerContainer(Container):
+    default_prefix = ""


No docstring here?

Tobychev · 2024-12-05T23:05:43Z

src/ctapipe/containers.py

+
+
+class SubarrayTriggerContainer(Container):
+    default_prefix = ""


No docstring here?

Tobychev · 2024-12-05T23:06:52Z

src/ctapipe/containers.py

@@ -773,12 +775,11 @@ class SimulatedCameraContainer(Container):
    )


-class SimulatedEventContainer(Container):
+class SimulationSubarrayContainer(Container):


No docstring here?

Tobychev · 2024-12-05T23:08:14Z

src/ctapipe/containers.py

-    array_altitude = Field(nan * u.rad, "Array pointing altitude", unit=u.rad)
-    array_ra = Field(nan * u.rad, "Array pointing right ascension", unit=u.rad)
-    array_dec = Field(nan * u.rad, "Array pointing declination", unit=u.rad)
+class SubarrayPointingContainer(Container):


No docstring here?

Tobychev · 2024-12-05T23:21:45Z

src/ctapipe/containers.py

    shower = Field(
        default_factory=SimulatedShowerContainer,
        description="True event information",
    )
-    tel = Field(default_factory=partial(Map, SimulatedCameraContainer))


With the tel removed this container now only contains one thing, so its not clear what the extra indirection step achieves.

Tobychev · 2024-12-05T23:27:24Z

src/ctapipe/containers.py

+        description="DL2 reconstructed event properties",
+    )
+
+    pointing = Field(


There is also pointing information in the tel field below, although it is per telescope rather than for the subarray.

Putting the subarray pointing info into the dl0 now seems strange, are there any quantities that depend on the subarray pointing at dl0? Isn't the telescope pointing the natural pointing until you do stereo?

This was linked to issues Jan 10, 2023

Discussion on restructuring Event Container structure #1165

Open

Rename *Camera*Containers to *Telescope*Containers #2170

Open

maxnoe force-pushed the container_structure branch from 9a4bb8e to 1e8ad14 Compare January 10, 2023 16:20

maxnoe requested a review from kosack January 11, 2023 08:28

maxnoe mentioned this pull request Jan 16, 2023

Rename Containers 'Camera' to 'Telescope' #2182

Closed

maxnoe changed the title ~~Start working on inverted container structure~~ Implement CEP-002: Change container structure to ArrayEvent -> TelescopeEvent -> Data Levels Jun 2, 2023

maxnoe force-pushed the container_structure branch from ce1d8ad to 352b041 Compare July 6, 2023 08:03

maxnoe force-pushed the container_structure branch from 352b041 to c522715 Compare July 7, 2023 10:25

kosack reviewed Jul 10, 2023

View reviewed changes

maxnoe mentioned this pull request Oct 21, 2023

Rename *Camera*Containers to *Telescope*Containers #2170

Open

maxnoe force-pushed the container_structure branch 2 times, most recently from 900249d to bd7d777 Compare October 31, 2023 16:57

maxnoe requested review from kosack, ccossou and Tobychev October 31, 2023 17:01

maxnoe commented Oct 31, 2023

View reviewed changes

ccossou reviewed Nov 7, 2023

View reviewed changes

ctapipe/image/image_processor.py Outdated Show resolved Hide resolved

maxnoe force-pushed the container_structure branch 2 times, most recently from 02e167c to 2a71a42 Compare April 30, 2024 13:17

maxnoe force-pushed the container_structure branch from 2a71a42 to 7022224 Compare April 30, 2024 13:29

This comment has been minimized.

Sign in to view

maxnoe commented Apr 30, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

maxnoe force-pushed the container_structure branch 2 times, most recently from e475e40 to e6719b0 Compare April 30, 2024 13:47

This comment has been minimized.

Sign in to view

Tobychev reviewed May 13, 2024

View reviewed changes

kosack previously approved these changes Jul 10, 2024

View reviewed changes

maxnoe dismissed kosack’s stale review via f24b07f July 12, 2024 14:02

maxnoe force-pushed the container_structure branch 4 times, most recently from f8939d0 to 7f603c4 Compare July 15, 2024 08:53

Use -C in makefile instead of cd

2cbfb48

maxnoe force-pushed the container_structure branch 3 times, most recently from bbecf66 to 1a17ad8 Compare July 15, 2024 11:37

Fix docs build

d7ef7b2

maxnoe force-pushed the container_structure branch from 1a17ad8 to 399bb24 Compare July 15, 2024 12:07

This comment has been minimized.

Sign in to view

maxnoe force-pushed the container_structure branch from 399bb24 to 8603de0 Compare July 15, 2024 12:47

Implement CEP-2: new event container structure

5474dca

maxnoe force-pushed the container_structure branch from 8603de0 to 5474dca Compare July 15, 2024 13:05

Tobychev reviewed Dec 5, 2024

View reviewed changes



		class SubarrayTriggerContainer(Container):
		default_prefix = ""

First step of implementing CEP-002: Change container structure #2204

Are you sure you want to change the base?

First step of implementing CEP-002: Change container structure #2204

Conversation

maxnoe commented Jan 10, 2023

maxnoe commented Jan 10, 2023

nbiederbeck commented Jan 11, 2023 • edited Loading

maxnoe commented Jun 5, 2023

maxnoe commented Jun 5, 2023

maxnoe commented Jun 5, 2023

StFroese commented Jul 7, 2023

maxnoe commented Jul 7, 2023

review-notebook-app bot commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StFroese commented Jul 10, 2023

maxnoe commented Oct 31, 2023

Choose a reason for hiding this comment

ccossou commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccossou commented Nov 7, 2023

maxnoe commented Apr 30, 2024

This comment has been minimized.

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

kosack left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ctao-dpps-sonarqube bot commented Jul 15, 2024

Analysis Details

12 Issues

Coverage and Duplications

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

nbiederbeck commented Jan 11, 2023 •

edited

Loading