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

dl1ab writing using ctapipe write_table #1111

Merged
merged 13 commits into from
Jun 13, 2023
Merged

Conversation

vuillaut
Copy link
Member

Follow up of #1031 using ctapipe read and write_table (ctapipe > v0.12 was required)

@vuillaut vuillaut changed the title dl1ab writing dl1ab writing using ctapipe write_table May 24, 2023
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 37.26% and project coverage change: -0.60 ⚠️

Comparison is base (7d2e3d0) 74.64% compared to head (ea651ef) 74.04%.

Additional details and impacted files
@@               Coverage Diff                @@
##           ctapipe_0.17    #1111      +/-   ##
================================================
- Coverage         74.64%   74.04%   -0.60%     
================================================
  Files               124      125       +1     
  Lines             12353    12523     +170     
================================================
+ Hits               9221     9273      +52     
- Misses             3132     3250     +118     
Impacted Files Coverage Δ
lstchain/calib/camera/pedestals.py 96.22% <ø> (ø)
lstchain/reco/r0_to_dl1.py 93.13% <ø> (ø)
lstchain/tools/lstchain_create_calibration_file.py 36.36% <0.00%> (-1.57%) ⬇️
lstchain/visualization/bokeh.py 15.74% <ø> (ø)
lstchain/scripts/lstchain_dl1ab.py 70.09% <37.31%> (-14.53%) ⬇️
...in/tools/lstchain_create_cat_B_calibration_file.py 38.01% <38.01%> (ø)
lstchain/calib/camera/calibration_calculator.py 92.00% <80.00%> (-0.08%) ⬇️
lstchain/calib/camera/flatfield.py 96.69% <100.00%> (ø)
lstchain/scripts/tests/test_lstchain_scripts.py 99.66% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gabemery
Copy link
Collaborator

gabemery commented Jun 1, 2023

Are these methods usable for the first dl1 file writing in r0_to_dl1?

@vuillaut
Copy link
Member Author

vuillaut commented Jun 1, 2023

Are these methods usable for the first dl1 file writing in r0_to_dl1?

It could be, but I don't want to consider this here and fix a working process (that should already be modified to use ctapipe-process).
The goal here was to solve an issue explained in #1031 while simplifying parameters writing in the dl1ab process.

@maxnoe
Copy link
Member

maxnoe commented Jun 1, 2023

Are these methods usable for the first dl1 file writing in r0_to_dl1?

r0_to_dl1 uses the ctapipe TableWriter, which produces the same output format as write_table, from containers instead of astropy tables. that is appropriate for r0_to_dl1, since it iterates over the events from the source.

@gabemery
Copy link
Collaborator

gabemery commented Jun 1, 2023

Ok, thanks for the clarifications

@rlopezcoto
Copy link
Contributor

is there anything missing with this PR or are you happy with the solutions implemented/answers @maxnoe, @gabemery ?

lstchain/io/lstcontainers.py Outdated Show resolved Hide resolved
@rlopezcoto
Copy link
Contributor

tests are not passing, any idea @vuillaut ?

@vuillaut vuillaut marked this pull request as draft June 7, 2023 16:08
@vuillaut
Copy link
Member Author

vuillaut commented Jun 7, 2023

tests are not passing, any idea @vuillaut ?

yep, working on it

@vuillaut vuillaut marked this pull request as ready for review June 8, 2023 16:51
@vuillaut
Copy link
Member Author

vuillaut commented Jun 9, 2023

Hi.
I integrated @maxnoe change request and merge ctapipe_0.17 branch. If you could have another look 🙏

Copy link
Contributor

@rlopezcoto rlopezcoto left a comment

Choose a reason for hiding this comment

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

I think this is good to go, if @maxnoe has time to have a final look, it would be great

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

LGTM

@rlopezcoto rlopezcoto merged commit 6cce4e6 into ctapipe_0.17 Jun 13, 2023
@rlopezcoto rlopezcoto deleted the dl1ab_writing branch June 13, 2023 07:35
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.

4 participants