Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept CEP 002 #2400

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Accept CEP 002 #2400

merged 1 commit into from
Jan 18, 2024

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Sep 20, 2023

Please use this PR to comment on a proposed ctapipe enhancement proposal (CEP): CEP-002: change the internal event structure.

Recall that ctapipe releases before 1.0.0 are subject to small and large changes in the API, and this is one such large change that has been discussed for several years now. Note that this affects any code that uses ctapipe, and therefore adopting it will require some changes to local code and scripts. This CEP has already been internally discussed in #2304, and is now open for public comment.

Please read the proposal for CEP-002 in the ctapipe docs and give constructive comments here if anything is ambiguous or missing.

Details about what is a CEP and how we incorporate them can be found in the documentation as CEP-001

@maxnoe
Copy link
Member

maxnoe commented Sep 22, 2023

Docs failure is fixed in main, please rebase

@mexanick
Copy link
Contributor

Can you please clarify how the events will be indexed and connected?
In the structure below, are the SubarrayEventIndex and TelescopeEventIndex independent? If yes, how the telescope-specific data shall be gathered to form a Subarray event, through tel:Map[...]?

SubarrayEvent
- index: SubarrayEventIndex
- simulation: SimulatedShower
- dl0: DL0Subarray
- dl1: DL1Subarray
- dl2: DL2Subarray
- dl3: DL3Subarray
- tel: Map[tel_id -> TelescopeEvent]

TelescopeEvent
- index: TelescopeEventIndex
- simulation: TelescopeSimulation
- r0: R0Telescope
- r1: R1Telescope
- dl0: DL0Telescope
- dl1: DL1Telescope
- dl2: DL2Telescope
- dl3: DL3Telescope

Also, with the new structure, will ctapipe-merge tool support merging of DL1 events (especially DL1a, required for Deep Learning models)?

@maxnoe
Copy link
Member

maxnoe commented Sep 25, 2023

In the structure below, are the SubarrayEventIndex and TelescopeEventIndex independent?

I don't understand whay you mean with "independent" here.

These two containers contain the "primary key"
identifying the subarray or telescope event respectively, i.e. obs_id / event_id for SubarrayIndexContainer and obs_id, event_id, tel_id for TelescopeIndexContainer.

If yes, how the telescope-specific data shall be gathered to form a Subarray event, through tel:Map[...]?

The key of .tel is the telescope id, as before and written in the snippet you posted.

Also, with the new structure, will ctapipe-merge tool support merging of DL1 events (especially DL1a, required for Deep Learning models)?

This CEP is concerned with the in-memory structure of the data (ctapipe containers), not the file structure.
So your question is independent of this CEP, but yes, we do plan to implement merging files per multiple telescopes into one file for the whole subarray, as this will also be how ACADA will take actual CTA data.

@Tobychev Tobychev added the documentation-only Label that will ensure code tests are skipped label Oct 26, 2023
@maxnoe
Copy link
Member

maxnoe commented Oct 31, 2023

If we want to merge this now, @kosack needs to remove the "draft" version of it we only kept so that the diff here renders nicely and not just "file renamed without changes".

@maxnoe maxnoe changed the title cep-002: Revised Event Structure (Acceptance Review) Accept CEP 002 and CEP 003 Nov 17, 2023
@maxnoe maxnoe force-pushed the cep2_acceptance branch 2 times, most recently from 3b5bdc4 to d801984 Compare November 24, 2023 11:26
@maxnoe maxnoe changed the title Accept CEP 002 and CEP 003 Accept CEP 002 Nov 24, 2023
@maxnoe maxnoe force-pushed the cep2_acceptance branch 2 times, most recently from 5b43b1c to 796becc Compare November 24, 2023 11:31
@maxnoe
Copy link
Member

maxnoe commented Nov 24, 2023

As discussed in today's meeting, I removed the CEP 3 from this PR and will open an individual PR for this.

So I think this is ready to be merged to formalize the acceptance.

maxnoe
maxnoe previously approved these changes Nov 24, 2023
@kosack kosack requested a review from Tobychev January 3, 2024 15:48
Tobychev
Tobychev previously approved these changes Jan 8, 2024
@maxnoe maxnoe dismissed stale reviews from Tobychev and themself via 9e6387c January 8, 2024 10:02
@maxnoe maxnoe requested review from maxnoe and Tobychev January 8, 2024 10:03
@kosack kosack merged commit 448fd62 into cta-observatory:main Jan 18, 2024
12 checks passed
@kosack kosack deleted the cep2_acceptance branch January 18, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-only Label that will ensure code tests are skipped no-changelog-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants