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

Fix tests for ctapipe_0.17 branch #1107

Merged
merged 4 commits into from
May 23, 2023
Merged

Conversation

chaimain
Copy link
Contributor

Forwarded from #1106 to have proper commit for the exact change. This will help solve some failing tests in #1054

@FrancaCassol
Copy link
Collaborator

Hi @chaimain, @maxnoe for solving the problem with the fit intensity scan one has to update the calibration files in the test_data adding. I do not know how to upload them, you find the correct ones in:
/fefs/aswg/workspace/franca.cassol/software/cta-lstchain/test_data/real/monitoring/PixelCalibration/Cat-A/calibration/20221001/ctapipe-v0.17/

@chaimain
Copy link
Contributor Author

Hi @chaimain, @maxnoe for solving the problem with the fit intensity scan one has to update the calibration files in the test_data adding. I do not know how to upload them, you find the correct ones in: /fefs/aswg/workspace/franca.cassol/software/cta-lstchain/test_data/real/monitoring/PixelCalibration/Cat-A/calibration/20221001/ctapipe-v0.17/

@maxnoe can update it I think. With these new files (and probably some more update on the code), do the tests pass for you locally?

@maxnoe
Copy link
Member

maxnoe commented May 23, 2023

I uploaded the new test files and update the path so that the tests use the new files, locally it worked already

@gabemery
Copy link
Collaborator

Do we want to fix the codacy issue in this PR too?
https://app.codacy.com/gh/cta-observatory/cta-lstchain/file/89587946300/issues/source?fileBranchId=32508679

(Unused line in the script create_test_calibration_files.sh)

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage: 85.02% and project coverage change: +2.18 🎉

Comparison is base (fd49d46) 71.73% compared to head (b20e8eb) 73.91%.

Additional details and impacted files
@@               Coverage Diff                @@
##           ctapipe_0.17    #1107      +/-   ##
================================================
+ Coverage         71.73%   73.91%   +2.18%     
================================================
  Files               121      123       +2     
  Lines             11306    11901     +595     
================================================
+ Hits               8110     8797     +687     
+ Misses             3196     3104      -92     
Impacted Files Coverage Δ
lstchain/scripts/lstchain_mc_sensitivity.py 24.52% <0.00%> (ø)
...site/onsite_create_calibration_files_with_batch.py 31.13% <ø> (ø)
...ain/scripts/onsite/onsite_create_drs4_time_file.py 38.46% <0.00%> (ø)
...tchain/tools/lstchain_create_drs4_pedestal_file.py 74.41% <ø> (-0.20%) ⬇️
lstchain/scripts/lstchain_longterm_dl1_check.py 5.17% <4.34%> (ø)
lstchain/tools/lstchain_create_calibration_file.py 37.93% <9.80%> (ø)
lstchain/visualization/bokeh.py 15.74% <13.33%> (ø)
lstchain/calib/camera/time_correction_calculate.py 42.59% <25.00%> (ø)
lstchain/scripts/lstchain_dl1_muon_analysis.py 31.74% <33.33%> (ø)
...n/scripts/onsite/onsite_create_calibration_file.py 25.90% <33.33%> (ø)
... and 47 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chaimain
Copy link
Contributor Author

I think the conflict issue of ctapipe_0.17 branch with master is separate from tests, and can be managed separately on that branch itself maybe. @FrancaCassol can you check it after we merge this?

@maxnoe maxnoe merged commit a96e0b1 into ctapipe_0.17 May 23, 2023
@maxnoe maxnoe deleted the ctapipe_0.17_fix_tests branch May 23, 2023 10:03
@FrancaCassol
Copy link
Collaborator

I think the conflict issue of ctapipe_0.17 branch with master is separate from tests, and can be managed separately on that branch itself maybe. @FrancaCassol can you check it after we merge this?

Hi @chaimain, I am not sure to understand what you ask to test, could please recall it to me?

@chaimain
Copy link
Contributor Author

chaimain commented May 23, 2023

I think the conflict issue of ctapipe_0.17 branch with master is separate from tests, and can be managed separately on that branch itself maybe. @FrancaCassol can you check it after we merge this?

Hi @chaimain, I am not sure to understand what you ask to test, could please recall it to me?

There were some conflicts in ctapipe_0.17 branch after your last PR #1071 was merged to master, and @maxnoe tried to address them #1054 (comment). Can you confirm whether everything is ok with the changes he made to those specific files (743cec2)?

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.

4 participants