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

Apply volume reduction to non gain selected waveforms #342

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

vuillaut
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #342 into master will decrease coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   43.67%   43.65%   -0.02%     
==========================================
  Files          69       69              
  Lines        4623     4625       +2     
==========================================
  Hits         2019     2019              
- Misses       2604     2606       +2     
Impacted Files Coverage Δ
lstchain/reco/volume_reducer.py 94.28% <60.00%> (-5.72%) ⬇️
lstchain/reco/r0_to_dl1.py 69.60% <100.00%> (ø)
lstchain/reco/tests/test_volume_reducer.py 95.74% <100.00%> (ø)

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 54a4e36...0e21b75. Read the comment docs.

@@ -75,7 +75,6 @@ def check_and_apply_volume_reduction(event, config):
pass

else:
print("Volume reduction algorithm being used: {}".format(volume_reduction_algorithm))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really against or in favor (just matter of tastes). But why taking out the stdout message ? It is true that to apply the vol_reduc, you must had consciously modified the config file... But this would be just a double check of what are you doing/applying int the analysis

Copy link
Member

@maxnoe maxnoe Apr 13, 2020

Choose a reason for hiding this comment

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

There shouldn't be any prints in the code base. If you want to keep this message, use the logging module (debug or info for this)

@@ -92,7 +91,12 @@ def check_and_apply_volume_reduction(event, config):

image[~pixels_to_keep] = 0
pulse_time[~pixels_to_keep] = 0
waveform[~pixels_to_keep, :] = 0
if waveform.ndim == 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still have cases in which we do not apply gain selection or this is just for testing purposes?

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually do not apply gain selection in real data currently (in the sense that we currently keep both channels).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sorry this is for R0 data

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, no DL0

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I meant before we apply gain selection (for DL1)

Copy link
Member Author

Choose a reason for hiding this comment

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

So if it's fine with you, I'd like to merge this

@vuillaut vuillaut merged commit 2873c08 into cta-observatory:master Apr 17, 2020
@vuillaut vuillaut deleted the volume_reduction branch April 17, 2020 13:14
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