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

Correct the search of interleaved files in the official tree #1192

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

FrancaCassol
Copy link
Collaborator

The present official tree path for DL1 files contains also the "short" code version: E.g.
/fefs/aswg/data/real/DL1/[date]/v0.10

This was not considered in the interleaved search function, this PR correct the search.

@FrancaCassol FrancaCassol added the bug Something isn't working label Dec 11, 2023
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

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

Comparison is base (107f8ce) 74.22% compared to head (404c8a2) 74.23%.
Report is 2 commits behind head on main.

Files Patch % Lines
...pts/onsite/onsite_create_cat_B_calibration_file.py 14.28% 6 Missing ⚠️
...nsite_create_cat_B_calibration_files_with_batch.py 14.28% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1192   +/-   ##
=======================================
  Coverage   74.22%   74.23%           
=======================================
  Files         130      130           
  Lines       13279    13273    -6     
=======================================
- Hits         9857     9853    -4     
+ Misses       3422     3420    -2     

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

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

I left some comments

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

Latest changes removing the version traitlet look fine to me.
My only concern is to add some tests to the Cat B calibration code if possible (to be done somewhere else)

@FrancaCassol
Copy link
Collaborator Author

Hi @morcuended, the tests of the code were added in other PRs, which type of tests do you mean? I can think to them for an other PR, this PR is just a bug correction.
@maxnoe could we merge this PR?

@maxnoe
Copy link
Member

maxnoe commented Jan 26, 2024

I can think to them for an other PR, this PR is just a bug correction.

The tests were passing before, the tests were unchanged and are still passing. Yet you say there was a bug. So the tests didn't cover the bug and we should add a new test that tests that it has been fixed correctly.

I.e. that test should fail before your code changes and pass after.

@morcuended
Copy link
Member

Hi @morcuended, the tests of the code were added in other PRs, which type of tests do you mean? I can think to them for an other PR, this PR is just a bug correction.

Ah, ok, I see that the calib B tool is 86% covered. The onsite calib B script is 30% covered.

The change in this PR is not really a bugfix. It depends on the directory tree in the container. You could assume the same directory tree in the tests as well

@moralejo moralejo merged commit 5b80be8 into main Mar 21, 2024
8 of 9 checks passed
@moralejo moralejo deleted the correct_dl1_search_path branch March 21, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants