Conversation
msilvafe
left a comment
msilvafe
left a comment
There was a problem hiding this comment.
Couple inline comments, feel free to push back/reply/resolve any and hit re-review when ready. I'll be fast next time around (I think)!
ykyohei
left a comment
ykyohei
left a comment
There was a problem hiding this comment.
Made suggested changes, thanks for the review.
Additionally, I modified bin_signal to accept arbitrary samps length of signal array. This won't affect previous bahavior and helpful for binning house keeping data which has different sampling rate from detector's data. Test for this new feature is also added test_400_arbitrary_length_of_signal
In addition to the unit test, tested this with real data and confirmed that this reproduces exact same results, when range is wide enough.
msilvafe
left a comment
msilvafe
left a comment
There was a problem hiding this comment.
Alright, thanks for addressing all of my previous comments. I'm approving but left a few inline comments which you can take-or-leave before merge.
2 participants
This is for binning 1d-array such as thermometer data to az.
This doesn't affect binning of 2d array.
There was an issue where, when the range was narrower than the bin_by value, the edge bins returned the wrong value due to np.digitize and np.clip.
This PR also fixes this issue. This error should not impact most analyses.
Added unit test for binning.