Added protection for rebin bin time #658
Closed
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
andLaing
force-pushed
the
protect-rebin
branch
2 times, most recently
from
5c18ea2 to
2696937
Compare
andLaing
force-pushed
the
protect-rebin
branch
from
2696937 to
7bb74d8
Compare
andLaing
force-pushed
the
protect-rebin
branch
2 times, most recently
from
9937bad to
f63afc8
Compare
gonzaponte
approved these changes
Nov 14, 2019
Collaborator
gonzaponte
left a comment
gonzaponte
left a comment
There was a problem hiding this comment.
Perfect! Good way to get rid of the negative weights that were messing up the order of the time bins.
andLaing
force-pushed
the
protect-rebin
branch
from
f63afc8 to
a35d28d
Compare
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
andLaing
force-pushed
the
protect-rebin
branch
2 times, most recently
from
420b37c to
1841241
Compare
Collaborator
|
Are you sure about this last commit? I don't think there is a huge difference in any case, but at first glance, the previous option seems less invasive, meaning that you only do something when it is actually needed. |
Collaborator
Author
|
Yes, sorry. I realised after the push that it was wrong and undid it. We're seeing some fail paths though due to an issue with the deconv and/or the selection of the peaks. I'mm looking at protections but I think it's a wider issue that should be addressed so I'll put an issue |
Collaborator
Author
|
I'm going to close this pull request as we found some failures in high statistics testing. The changes plus additional protections are revisited in PR #683 |
Merged
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
2 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.
Only uses positive charge for
weighting to avoid non-sequential
bins.