cta-observatory / ctapipe_io_nectarcam

ctapipe plugin for reading nectarcam files
BSD 3-Clause "New" or "Revised" License
2 stars 11 forks source link

Modifications for EVB v6 #51

Closed vmarandon closed 5 months ago

vmarandon commented 8 months ago

Modifications to be able to read data from the EVB v6 while keeping the capability to read old files.

A storage class was added : NectarCAMDataStreamContainer which contain the information from TelescopeDataStream. The scaling factors that are apply to the data imply a change of type from uint16 to float for the R0. It should be transparent though (and only done for v6 data)

A little bit of refactoring was done to avoid to have if/else at different place (so use the NectarCAMServiceContainer instead of the camera_config when possible)

I'm pinging also @sizun to have a look (or if he wants to test this branch) as it seems I can't assign him for the review.

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 67.36111% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 84.08%. Comparing base (60533fe) to head (50bff9b).

:exclamation: Current head 50bff9b differs from pull request most recent head af2e25e

Please upload reports for the commit af2e25e to get more accurate results.

Files Patch % Lines
src/ctapipe_io_nectarcam/__init__.py 63.56% 47 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #51 +/- ## ========================================== - Coverage 88.75% 84.08% -4.68% ========================================== Files 7 7 Lines 685 729 +44 ========================================== + Hits 608 613 +5 - Misses 77 116 +39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jlenain commented 5 months ago

I reverted to the last commit by @vmarandon , since I messed up today when trying to resolve conflicts with main.

The main conflict concerns keeping the LightNectarCAMEventSource and NectarCAMEventSource classes in the source code, as well as in unit tests (see PR #47 )

vmarandon commented 5 months ago

I've merged the branch with the master according to the change in LightNectarCAMEventSource discussed on slack. The conflict seems to be solved but would be nice if you could have a look to see if for you everything looks ok. (I've just been through it and seems ok to me) If so, we should then merge the pull request.

jlenain commented 5 months ago

Thanks a lot, @vmarandon ! I very much like your re-implementation of LightNectarCAMEventSource, which is super elegant.

I also went through your last changes, and tested everything locally. I just pushed a small commit increasing slightly the coverage on tests for the light version of the event source.

If the CI tests pass, all would seem good to me too for merging this PR.