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

Refactoring of GainSelector #1093

Merged
merged 2 commits into from
Jun 21, 2019
Merged

Conversation

watsonjj
Copy link
Contributor

This PR is for the refactoring and discussion of the existing (but mostly unused) GainSelector classes.

My next PR will be dedicated to the integration of this class in the calibration chain.

I have simplified the GainSelector class in a similar way as was recently performed for the ImageExtractor class in design. It acts as a low-level class which operates on the waveforms numpy array.

This PR picks up from #683, and includes the suggestion from #818. Other related issues include: #419, #962 and #822.

@watsonjj watsonjj added this to the v0.7 milestone Jun 20, 2019
@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #1093 into master will decrease coverage by 0.06%.
The diff coverage is 98.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
- Coverage   84.35%   84.29%   -0.07%     
==========================================
  Files         182      182              
  Lines       11056    11006      -50     
==========================================
- Hits         9326     9277      -49     
+ Misses       1730     1729       -1
Impacted Files Coverage Δ
ctapipe/calib/camera/tests/test_gainselection.py 100% <100%> (ø) ⬆️
ctapipe/calib/camera/gainselection.py 96.42% <96.15%> (+0.77%) ⬆️

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 a87947f...b49f4a9. Read the comment docs.

Copy link
Contributor

@thomasarmstrong thomasarmstrong left a comment

Choose a reason for hiding this comment

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

Looks good to me, only one question but only for curiosity.

return waveforms[0]
else:
pixel_channel = self.select_channel(waveforms)
return waveforms[pixel_channel, np.arange(n_pixels)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question, why not just 'return waveforms[pixel_channel, :]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately is isn't as simple to index the array like that. waveforms[pixel_channel, :] returns an array of shape (n_pix, n_pix, n_samples).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(When pixel_channel is an array of n_pix itself)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok

@watsonjj watsonjj merged commit fdd3032 into cta-observatory:master Jun 21, 2019
@kosack
Copy link
Contributor

kosack commented Jun 21, 2019

I just remembered that it's also necessary to keep the mask of which channel was used as well as the gain-selected waveform. Perhaps that could be done in another PR.

@vuillaut
Copy link
Member

I just remembered that it's also necessary to keep the mask of which channel was used as well as the gain-selected waveform. Perhaps that could be done in another PR.

+1

@watsonjj Are you working on a fixing PR right now?
This PR broke lstchain with no easy way to fix it without the pixel_channel mask.

@watsonjj watsonjj deleted the gain_selection branch August 12, 2019 11:32
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.

4 participants