Protect rebin #687
Merged
Protect rebin #687
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Only uses positive charge for weighting to avoid non-sequential bins.
Data had a peak affected by -ve charges data updated to represent new expectation
mmkekic
reviewed
Dec 18, 2019
Collaborator
mmkekic
left a comment
mmkekic
left a comment
There was a problem hiding this comment.
The 2 tests show ill-behaved old version of rebinning algorithm and the need to fix the bugs. The fix ensures that the bin position after rebinning will be physical. This PR still doesn't fix the problem of negative waveforms after the rebinning (this was intended in PR #683) , as I understand this is left to the future PR of @ausonandres and @msorel.
mmkekic
reviewed
Dec 19, 2019
andLaing
force-pushed
the
protect-rebin
branch
from
7a95d65 to
d45b1e3
Compare
mmkekic
approved these changes
Dec 28, 2019
Collaborator
mmkekic
left a comment
mmkekic
left a comment
There was a problem hiding this comment.
This PR demonstrates and fixes the bug in weighted rebinning of the waveforms with negative weights. The change is minimal and clear, and the test are relevant for any kind of future refactoring of irene city. Good job!
carmenromo
added a commit
that referenced
this pull request
Dec 31, 2019
#687 [author: andLaing] This PR reopens #658 with an added patch to protect against attempts to rebin using only negative charge bins. Tests are added to demonstrate the problems and their solution. [reviewer: mmkekic] This PR demonstrates and fixes the bug in weighted rebinning of the waveforms with negative weights. The change is minimal and clear, and the test are relevant for any kind of future refactoring of irene city. Good job!
andLaing
pushed a commit
to andLaing/IC
that referenced
this pull request
May 13, 2020
next-exp#687 [author: andLaing] This PR reopens next-exp#658 with an added patch to protect against attempts to rebin using only negative charge bins. Tests are added to demonstrate the problems and their solution. [reviewer: mmkekic] This PR demonstrates and fixes the bug in weighted rebinning of the waveforms with negative weights. The change is minimal and clear, and the test are relevant for any kind of future refactoring of irene city. Good job!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reopens #658 with an added patch to protect
against attempts to rebin using only negative charge bins.
Tests are added to demonstrate the problems and their
solution.