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

Add StatisticsAggregator API #2554

Merged
merged 61 commits into from
Aug 2, 2024
Merged

Add StatisticsAggregator API #2554

merged 61 commits into from
Aug 2, 2024

Conversation

TjarkMiener
Copy link
Member

@TjarkMiener TjarkMiener commented May 2, 2024

This PR adds a skeleton API for the statistical calculation of pixel-wise values over a given time interval. The current implementation features two different extraction methods, i.e. a PlainExtractor based on numpy functions and SigmaClippingExtractor based on astropy functions, which are also used in lstchain.

I'm marking this PR as a draft because of the following discussion points:

  1. How to implement the unit tests? I would propose to create random distributions with a given mean and std to assert the correctness of the extraction components.
  2. How to deal with the last data chunk that is calculated with fewer samples as requested? Skip it or take the last samples fulfilling the sample_size?
  3. Allow overlapping chunks of samples? How to define the validity range (using start-stop or only start)?

Related to #2542.

src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/calib/camera/tests/test_extractors.py Outdated Show resolved Hide resolved
src/ctapipe/containers.py Outdated Show resolved Hide resolved
@TjarkMiener
Copy link
Member Author

I'm having some troubles with ruff, which makes pylint fail. Can someone please help? Thanks in advance!

mexanick
mexanick previously approved these changes Jun 12, 2024
@maxnoe
Copy link
Member

maxnoe commented Jun 12, 2024

For future reference: the issue was that the pre-commit hook wasn't yet installed for some of the commits.

To run it on all files affected by this PR, the solution was:

pre-commit run --files $(git diff --name-only origin/main)

This comment has been minimized.

@TjarkMiener TjarkMiener marked this pull request as ready for review June 12, 2024 13:54
mexanick
mexanick previously approved these changes Jun 13, 2024
ctoennis
ctoennis previously approved these changes Jun 13, 2024
@TjarkMiener TjarkMiener dismissed stale reviews from ctoennis and mexanick via 5204662 June 17, 2024 08:49

This comment has been minimized.

@TjarkMiener TjarkMiener dismissed stale reviews from mexanick and maxnoe via 5a143dd July 31, 2024 14:32
kosack
kosack previously approved these changes Jul 31, 2024
@kosack
Copy link
Contributor

kosack commented Jul 31, 2024

We can generalize it more later, but for now this is fine

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.

monitoring/extractor.py is a new module, it needs to be added to the API docs.

Please add it in docs/api-reference/monitoring/ .

You can check the other files there

@TjarkMiener
Copy link
Member Author

Please add it in docs/api-reference/monitoring/ .

I tried to added following the structure of the other modules, but the docs are failing. Can you please have a look @maxnoe? Thanks in advance!

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2024

The error is not in the new docs page (these are fine!) but in the docstring of one of the new classes:

 /home/runner/work/ctapipe/ctapipe/src/ctapipe/monitoring/extractor.py:docstring of ctapipe.monitoring.extractor.StatisticsExtractor.__call__:4: WARNING: py:obj reference target not found: chunk_shift
/home/runner/work/ctapipe/ctapipe/src/ctapipe/monitoring/extractor.py:docstring of ctapipe.monitoring.extractor.StatisticsExtractor.__call__:4: WARNING: py:obj reference target not found: chunk_shift

which are now included in the docs, which is why you didn't see them before.

You need double back-ticks in rst for code:

``chunk_shift``

maxnoe
maxnoe previously approved these changes Aug 1, 2024
src/ctapipe/monitoring/extractor.py Outdated Show resolved Hide resolved
@TjarkMiener TjarkMiener changed the title Add StatisticsExtractor API Add StatisticsAggregator API Aug 2, 2024
@maxnoe maxnoe merged commit 447f44c into main Aug 2, 2024
12 checks passed
@maxnoe maxnoe deleted the stats_extractor branch August 2, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants