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

Refactor/rename data writer options #1920

Closed

Conversation

kosack
Copy link
Contributor

@kosack kosack commented May 25, 2022

To be merged after #1899

rename :

  • write_showers → write_dl2
  • write_parameters → write_dl1_parameters
  • write_images → write_dl1_images
  • write_raw_waveforms → write_r0_waveforms
  • write_waveforms → write_r1_waveforms

- added tests comparing to using a Frame transform
- created alternate implementation for test comparison
- use az -> -az to make azimuth go in the correct direction
- spherical_to_cartesian
- thanks to Julian S for spotting this!
updated all places where they are used as well
Also issubset was used in the wrong order. Not sure how this worked before...

The array in the test files is:

```
Subarray : MonteCarloArray
Num Tels : 180
Footprint: 4.92 km2

       Type       Count     Tel IDs
----------------- ----- ---------------
   LST_LST_LSTCam     4 1-4
MST_MST_NectarCam    28 100-124,128-130
   SST_ASTRI_CHEC   120 30-99,131-180
 MST_MST_FlashCam    28 5-29,125-127
```
instead of to a separate impact table
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1920 (849d69f) into master (d02aad7) will increase coverage by 0.03%.
The diff coverage is 97.01%.

@@            Coverage Diff             @@
##           master    #1920      +/-   ##
==========================================
+ Coverage   92.08%   92.12%   +0.03%     
==========================================
  Files         189      191       +2     
  Lines       14954    15053      +99     
==========================================
+ Hits        13771    13868      +97     
- Misses       1183     1185       +2     
Impacted Files Coverage Δ
ctapipe/conftest.py 94.47% <ø> (ø)
ctapipe/io/tests/conftest.py 100.00% <ø> (ø)
ctapipe/io/tests/test_datawriter.py 100.00% <ø> (ø)
ctapipe/tools/tests/test_merge.py 97.18% <ø> (ø)
ctapipe/io/tableloader.py 94.33% <84.61%> (-0.90%) ⬇️
ctapipe/io/datawriter.py 89.76% <90.90%> (+0.03%) ⬆️
ctapipe/containers.py 100.00% <100.00%> (ø)
ctapipe/coordinates/__init__.py 100.00% <100.00%> (ø)
ctapipe/coordinates/tests/test_coordinates.py 100.00% <100.00%> (ø)
ctapipe/io/simteleventsource.py 95.02% <100.00%> (+0.08%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d02aad7...849d69f. Read the comment docs.

maxnoe
maxnoe previously approved these changes May 25, 2022
@maxnoe
Copy link
Member

maxnoe commented Feb 23, 2024

I think this would still be nice to have, could you rebase on current main?

@kosack
Copy link
Contributor Author

kosack commented Mar 4, 2024

Moved to #2520

@kosack kosack closed this Mar 4, 2024
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.

None yet

2 participants