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 calibration calculators #2609

Merged
merged 116 commits into from
Oct 24, 2024
Merged

Add calibration calculators #2609

merged 116 commits into from
Oct 24, 2024

Conversation

TjarkMiener
Copy link
Member

@TjarkMiener TjarkMiener commented Aug 26, 2024

This PR contains the StatisticsCalculator, which aggregates statistics, detects outliers, handles faulty data chunks. The StatisticsCalculator holds two functions to conduct two different passes over the data with and without overlapping chunks. The first pass is conducted with non-overlapping, while overlapping chunks can be set by the chunk_shift parameter in the second pass. The second pass over the data is only conducted in regions of trouble with a high percentage of faulty pixels exceeding the threshold faulty_pixels_threshold.

related to issue #2542

Tjark Miener and others added 30 commits April 29, 2024 08:51
added PlainExtractor based on numpy and scipy functions
restructured the stats containers
Remove StarVarianceExtractor since is functionality is featured in the existing Extractors
allow overlapping extraction sequences
renaming to chunk(s) and chunk_size and _shift

added test for chunk_shift and boundary case

This comment has been minimized.

@maxnoe
Copy link
Member

maxnoe commented Oct 22, 2024

Correct, we have prototype tools ready in calibpipe-MRs for the stats calculation and the ffactor method.

I think (after this is merged here) we should talk about the place of these tools.

I don't really see why we should add the basic components needed (only) by these tools to ctapipe itself, but then the tools only to calibpipe.

renamed traits in the tests for the outlier detection

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 97.30% Coverage (93.70% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

Copy link
Contributor

@kosack kosack 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 it looks good now. The only question is if you should add a template/example config file for it that can be generated by ctapipe-quickstart, as we do for other tools.

For that you would just need to create a YAML config file in ctapipe/reasources/ and modify CONFIGS_TO_WRITE in quickstart.py.

@maxnoe
Copy link
Member

maxnoe commented Oct 24, 2024

@kosack there is no tool here

@kosack
Copy link
Contributor

kosack commented Oct 24, 2024

@kosack there is no tool here

You're right... I was looking at the config in the test case, but that is only a Component config, and the tool is in calibpipe.

Then everything is ok, and we can open an issue to add a generic tool separately.

@maxnoe
Copy link
Member

maxnoe commented Oct 24, 2024

@TjarkMiener If you have no issue with it, I will squash this PR. 116 commits for these changes seems a bit much.

@TjarkMiener
Copy link
Member Author

Sure, I think the huge numbers of commits is because we branched off the stats_extractor branch back in the days, which are included.

@maxnoe maxnoe merged commit fa4dac6 into main Oct 24, 2024
13 checks passed
@maxnoe maxnoe deleted the CalibrationCalculators branch October 24, 2024 14:29
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.

4 participants