-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add c++ jump fixing and finding of slow jumps #196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Only a single comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy to see this!
Please add smoke tests for these functions.
I would really like you to loosen the requirement on strides[0]. But I suppose it's not urgent for me if it's not urgent for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I have any major comments outside of one quick question.
for (auto const &r: ranges[di].segments) { | ||
start = di*nsamp + r.first; | ||
for(int j = stop; j < start && to_sub != 0; j++) { | ||
output[j] = tod_data[j] - to_sub; | ||
} | ||
stop = di*nsamp + r.second; | ||
min_h = *(std::min_element(jump_heights+start, jump_heights+stop)); | ||
max_h = *(std::max_element(jump_heights+start, jump_heights+stop)); | ||
// Decide whether this is a negative or positive jump. | ||
height = (abs(min_h) > abs(max_h)) ? min_h : max_h; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the answer is yes, but is this okay if there is a jump at the first or last samples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thats fine, the mean level will shift to around the first sample's value in that case, but that should be ok since we will want to mean sub after gapfilling anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I am only looking the other way on the bulk-reformatting of that test file because it is well-separated from where you made actual changes.
Remember to squash on/before merge.
Stupid diffs giving away my love for black... |
Still a draft because my so3g build system is broken rn...