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

Ctar1 fixes #199

Merged
merged 20 commits into from
Nov 10, 2023
Merged

Ctar1 fixes #199

merged 20 commits into from
Nov 10, 2023

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Sep 28, 2023

Changes to make it fully work with EVBv6 data, not yet ready, will update as more test data is taken

Missing:

  • Read info on event-preprocessing done by EVB
  • Apply only needed steps
  • Missing module handling in code branch for new data format data (do we need this?)

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (273ec6d) 89.48% compared to head (24e6138) 89.42%.
Report is 2 commits behind head on main.

❗ Current head 24e6138 differs from pull request most recent head e868560. Consider uploading reports for the commit e868560 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   89.48%   89.42%   -0.06%     
==========================================
  Files          20       21       +1     
  Lines        2245     2346     +101     
==========================================
+ Hits         2009     2098      +89     
- Misses        236      248      +12     
Files Coverage Δ
src/ctapipe_io_lst/multifiles.py 91.97% <100.00%> (+0.17%) ⬆️
src/ctapipe_io_lst/pointing.py 90.44% <100.00%> (+0.21%) ⬆️
src/ctapipe_io_lst/tests/test_calib.py 100.00% <100.00%> (ø)
src/ctapipe_io_lst/tests/test_cta_r1.py 100.00% <100.00%> (ø)
src/ctapipe_io_lst/tests/test_pointing.py 100.00% <100.00%> (ø)
src/ctapipe_io_lst/evb_preprocessing.py 95.65% <95.65%> (ø)
src/ctapipe_io_lst/__init__.py 88.95% <95.71%> (+0.33%) ⬆️
src/ctapipe_io_lst/calibration.py 62.24% <52.94%> (-0.72%) ⬇️

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

@maxnoe maxnoe marked this pull request as ready for review October 25, 2023 08:33
@maxnoe
Copy link
Member Author

maxnoe commented Oct 30, 2023

This PR is already pretty large and is enough to get analysis working of data taken with EVBv6 without corrections applied or with manually specifying which corrections have already been applied.

Could you please review @FrancaCassol @moralejo or @SeiyaNozaki ?

I'll open another PR for the missing module and the automatic detection of calibration steps already performed by EVB

Copy link
Contributor

@moralejo moralejo left a comment

Choose a reason for hiding this comment

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

Had a look at seems ok to me, but better if sb else checks it too...

offset = self.data_stream.waveform_offset
pixel_id_map = self.camera_config.pixel_id_map

# FIXME: missing modules / pixels
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will not work yet with data with missing modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not yet, see my comment above.

I'd like to separate that part into a second PR, with proper dummy test data and so on. This one will get pretty large if I want to include everything.

@maxnoe maxnoe merged commit e0fccfb into main Nov 10, 2023
6 of 7 checks passed
@maxnoe maxnoe deleted the ctar1_fixes branch November 10, 2023 08:57
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.

2 participants