Skip to content

Conversation

@vmarandon
Copy link
Contributor

Will read the data per block instead of all the files together. This remove the limitation of long acquisition that consume too much memory and reach quickly the limit of opened file.

The grouping of the blocks is 4 by default and correspond to the number of data stream of the acquisition.

There is also option to only read a certain set of blocks

@codecov
Copy link

codecov bot commented Sep 15, 2024

Codecov Report

Attention: Patch coverage is 61.41732% with 49 lines in your changes missing coverage. Please review.

Project coverage is 85.60%. Comparing base (60533fe) to head (cb5646d).
Report is 58 commits behind head on main.

Files with missing lines Patch % Lines
src/ctapipe_io_nectarcam/__init__.py 61.41% 49 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
- Coverage   88.75%   85.60%   -3.16%     
==========================================
  Files           7        7              
  Lines         685      917     +232     
==========================================
+ Hits          608      785     +177     
- Misses         77      132      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlenain
Copy link
Contributor

jlenain commented Oct 8, 2024

Hi @vmarandon !

Thanks a lot for that!

It would be good indeed if the new class BlockNectarCAMEventSource could inherit from NectarCAMEventSource.

…ub files when there is missing file instead of using the normal behavior or NectarCAMEventSource
@vmarandon vmarandon marked this pull request as ready for review January 14, 2025 17:41
@vmarandon
Copy link
Contributor Author

As I'm using the NectarCAMEventSource as the baseline for the processing of each block, I don't think it's possible to make it inherit from NectarCAMEventSource as it would imply some weird inheritance diagram.
I think the whole ctapipe_io_nectarcam would need to be re-organized/re-written to do so.
A possibility would be to make the BlockNectarCAMEventSource the "main" class by transferring the is_compatible static method to it so that this would be the class seen by EventSource. I'm not sure this class is ready for it as I'm the only one using it at the moment... 😅

@jlenain jlenain self-requested a review January 17, 2025 13:19
@jlenain jlenain added the enhancement New feature or request label Jan 17, 2025
@jlenain
Copy link
Contributor

jlenain commented Jan 17, 2025

Hi @vmarandon ,

Many thanks for this PR! Do you also have an implementation of unit tests ready for the new BlockNectarCAMEventSource class ?
My guess is that it should as easy as just adding it to the list of event sources used in https://github.com/cta-observatory/ctapipe_io_nectarcam/blob/main/src/ctapipe_io_nectarcam/tests/test_nectarcameventsource.py.

@vmarandon
Copy link
Contributor Author

I've expanded the existing test to add BlockNectarCamEventSource.
I've also made some minor change to the class documentation and added a is_compatible function that return False.

Copy link
Contributor

@jlenain jlenain left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @vmarandon , also for adding unit tests for BlockNectarCAMEventSource.

@jlenain jlenain merged commit 6f6dd62 into cta-observatory:main Jan 20, 2025
4 of 6 checks passed
@jlenain jlenain deleted the blocksource branch January 20, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants