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

Switch back to peakfinding in the matched filter #807

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

skhrg
Copy link
Member

@skhrg skhrg commented Apr 15, 2024

Realized that in some round of modifications I essentially canceled out the matched filter since I was looking for peaks in np.diff(np.cumsum(x), 2) which is basically just np.diff(x) (there is some smoothing as well).

This is because np.cumsum(x) makes jumps show up as slope changes unless we cross 0. In the old recursive version this was solved by mean subtracting and finding the largest jump (which crossed 0 when mean subbed) and then recursively repeating between jumps which was slow.

Here we solve the problem by detrending the matched filtered data which makes the slope changes look like relative extrema and then using argrelmax (much faster than find_peaks) to look for them. EDIT: Actually the detrend to make things peaks doesn't work as well as I like, what does work well is to subtract off a mean template.

Tested this base algorithm on some LAT data and it seems more robust to false positives (especially glitches).

@skhrg skhrg requested review from kmharrington and removed request for kmharrington April 15, 2024 03:26
@skhrg skhrg marked this pull request as draft April 15, 2024 03:52
@skhrg skhrg force-pushed the jump_peaks branch 2 times, most recently from 5fe973c to 92a2de8 Compare April 15, 2024 06:22
@msilvafe
Copy link
Contributor

Could you make a wiki log post that summarizes the core of this PR? Like a description in words (and/or math) of what the different algorithms (current + this PR) are doing, which false positives you were finding with the old algorithm and what those samples look like with this PR for a few representative cases, and some summary comparisons between the two (time to run, memory to run, number of glitches found, etc.)?

@skhrg
Copy link
Member Author

skhrg commented Apr 15, 2024

Yeah that sounds like a good idea is this the correct place to put it? https://simonsobs.atlassian.net/wiki/spaces/PRO/pages/236421160/TOD+Processing+Analysis+Logbook

I kinda think a generic page under https://simonsobs.atlassian.net/wiki/spaces/PRO/pages/236421121/1.06.07.05+TOD+Processing like "[TOD Processing] Algorhitm Documentation" with a page for each operation (jump flag, glitch flag, etc.) where we can stick explanations of math and stuff might be nice but maybe I'm getting ahead of myself.

@msilvafe
Copy link
Contributor

Yeah that sounds like a good idea is this the correct place to put it? https://simonsobs.atlassian.net/wiki/spaces/PRO/pages/236421160/TOD+Processing+Analysis+Logbook

I kinda think a generic page under https://simonsobs.atlassian.net/wiki/spaces/PRO/pages/236421121/1.06.07.05+TOD+Processing like "[TOD Processing] Algorhitm Documentation" with a page for each operation (jump flag, glitch flag, etc.) where we can stick explanations of math and stuff might be nice but maybe I'm getting ahead of myself.

Either is fine with me.

@skhrg skhrg force-pushed the jump_peaks branch 2 times, most recently from 863ae90 to 6c9b30a Compare April 16, 2024 21:28
@skhrg
Copy link
Member Author

skhrg commented May 2, 2024

Ok see this for details but I think I am happy with this PR and it is a safe move that helps performance and is easier to understand.

@skhrg skhrg marked this pull request as ready for review May 2, 2024 20:04
@skhrg skhrg requested a review from msilvafe May 2, 2024 20:04
@skhrg
Copy link
Member Author

skhrg commented May 23, 2024

Ok this now also includes speed ups in C++ for twopi jumps.
I'd like to also move height estimation jumpfixing to C++, but I think that can go in a separate PR since this stuff offers a more immediate improvement.

@skhrg skhrg force-pushed the jump_peaks branch 2 times, most recently from ab61e03 to 51953aa Compare May 30, 2024 23:02
@skhrg skhrg force-pushed the jump_peaks branch 2 times, most recently from ce75039 to 4a640f7 Compare August 27, 2024 06:25
* so3g block_moment
* matched filter in so3g
* 2pi jumps in so3g
* Faster diff buffed
* Switch to mean sub
* Do more things inplace
* Remove unnneeded std check
@skhrg
Copy link
Member Author

skhrg commented Aug 27, 2024

Ok tests pass now that we have the new so3g build! @mhasself what do I need to do to get this thing merged?

@skhrg skhrg requested a review from mhasself September 22, 2024 01:44
@msilvafe msilvafe mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants