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

Remove dump-triggers, enable ctapipe-process to store only trigger and simulation data #2375

Merged
merged 2 commits into from Jul 6, 2023

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jul 5, 2023

fixes #2372

Running ctapipe-process -i <input> -o <output> --no-write-parameters will result in an output file only containing trigger and simulation information, no any actual processing will happen.

@maxnoe maxnoe requested review from kosack and mdpunch July 5, 2023 12:48
kosack
kosack previously approved these changes Jul 5, 2023
@kosack
Copy link
Contributor

kosack commented Jul 5, 2023

i didn't check, but I guess the merge tool will work with these files, since this info was already merged?

@maxnoe
Copy link
Member Author

maxnoe commented Jul 5, 2023

@kosack Yes, merging works fine

Comment on lines 616 to 629
def _write_trigger(self, writer: TableWriter, event: ArrayEventContainer):
"""
Write trigger information
"""
self._writer.write(
table_name="dl1/event/subarray/trigger",
containers=[event.index, event.trigger],
)

for tel_id, trigger in event.trigger.tel.items():
writer.write(
"dl1/event/telescope/trigger", (_get_tel_index(event, tel_id), trigger)
)

Copy link
Member

@StFroese StFroese Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question for my understanding.
self._write_trigger(self._writer, event) is called in L304, which means
writer=self._writer.
But self._writer is already available inside the function.
In which case do you want to use a different writer with this function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely a leftover from the original design of the DataWriter, where I had considered having a writer per table. There may have been some other reason for that design as well, but I don't recall. However, currently only one is used as TableWriter supports writing to multiple tables. We can refactor that later, but I agree here at least you should use writer.write() and not self._writer.write()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could have been because DataWriter was meant to support other types of TableWriter in the future as well (only HDF5 is currently implemented, but it would be pretty easy to have other formats). But even then, we could assume the TableWriter itself supports handling of N tables.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a copy paste mistake, I moved the subarray writing from call where it directly accessed self._writer

Will change.

Since this is all internal anyway, I could also just remove the wrote argument though

mdpunch
mdpunch previously approved these changes Jul 5, 2023
Copy link

@mdpunch mdpunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, thanks!

@maxnoe maxnoe dismissed stale reviews from mdpunch and kosack via ea996fe July 6, 2023 07:43
@maxnoe
Copy link
Member Author

maxnoe commented Jul 6, 2023

Removed the superflous writer= argument from the internal functions, please review agan @kosack @mdpunch

@kosack kosack merged commit c44226a into main Jul 6, 2023
12 of 13 checks passed
@kosack kosack deleted the only_triggers branch July 6, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ctapipe-dump-triggers
4 participants