Skip to content

Conversation

@FrancaCassol
Copy link
Collaborator

This is necessary in order to move to fits output as requested by EvB

@FrancaCassol FrancaCassol requested a review from maxnoe March 7, 2025 12:37
@codecov
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.03%. Comparing base (0bfa981) to head (c0a19ac).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/ctapipe_io_lst/calibration.py 94.11% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
+ Coverage   90.97%   91.03%   +0.05%     
==========================================
  Files          25       25              
  Lines        2637     2688      +51     
==========================================
+ Hits         2399     2447      +48     
- Misses        238      241       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maxnoe
Copy link
Member

maxnoe commented Mar 25, 2025

You need to rebase or merge with main.

This also needs tests with the fits files produced by lstcam-calib.

@FrancaCassol
Copy link
Collaborator Author

You need to rebase or merge with main.

it tells it is already up to date

def test_to_native():
from ctapipe_io_lst.calibration import to_native

assert to_native(np.array([1, 2], dtype='>i4')).dtype=='int32'
Copy link
Member

@maxnoe maxnoe Mar 25, 2025

Choose a reason for hiding this comment

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

This only tests the case where the array is not yet native and it also doesn't verify that the values are actually correct.

@maxnoe
Copy link
Member

maxnoe commented Mar 25, 2025

You need to rebase or merge with main.

it tells it is already up to date

Ah, sorry, right. The fix for the CI is not yet merged, this PR is needed, please review:
#237

@FrancaCassol
Copy link
Collaborator Author

This also needs tests with the fits files produced by lstcam-calib.

@maxnoe, I need to add fits calibration and drs4 baseline files, but it seems that that the other test data are in lstchain, what do you suggest to do?

@maxnoe maxnoe force-pushed the read_of_fits_calibration_files branch from 0e88a35 to 5188636 Compare March 26, 2025 12:11
@FrancaCassol
Copy link
Collaborator Author

I decided at the end to create only one test for all parameters

@maxnoe maxnoe merged commit 114fce8 into main Mar 27, 2025
7 of 8 checks passed
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.

3 participants