Skip to content

Whitening Filter Threshold #1125

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Jul 23, 2024
Merged

Whitening Filter Threshold #1125

merged 21 commits into from
Jul 23, 2024

Conversation

@j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Jun 6, 2024

The current implementation of whiten uses a slightly different threshold for zeroing out filter values when dividing by small numbers.

This PR implements makes this value configurable with the same default threshold as matlab, namely sqrt(psd) < 100 * eps(src.dtype).

@j-c-c j-c-c added the cleanup label Jun 6, 2024
@j-c-c j-c-c self-assigned this Jun 6, 2024
@j-c-c j-c-c force-pushed the whiten_threshold branch from e9837bc to 8dc720d Compare June 18, 2024 13:19
@j-c-c j-c-c requested a review from garrettwrong June 18, 2024 16:01
garrettwrong
garrettwrong requested changes Jun 18, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a crack at this. Two remarks are just follow up from yesterday, probably something was wrong with my suggestion, just lmk, thanks.

src/aspire/operators/filters.py Outdated

return result
# Recast result with correct dtype
return result.astype(self.xfer_fn_array.dtype)
Copy link
Collaborator

@garrettwrong garrettwrong Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other remark, but if we still need this, lets make it copy=False. Thanks

Copy link
Collaborator Author

@j-c-c j-c-c Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this (see comment below). I will make it copy=False. Thanks.

garrettwrong
garrettwrong previously approved these changes Jun 20, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, like the test. Thank you.

@garrettwrong
Copy link
Collaborator

garrettwrong commented Jun 20, 2024

Saw the unit test fail on the new platform. If it keeps coming up I'll look at it more. I don't think it has anything to do with your changes so I restarted it for you.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Jun 20, 2024

Saw the unit test fail on the new platform. If it keeps coming up I'll look at it more. I don't think it has anything to do with your changes so I restarted it for you.

This actually happened once before: https://github.com/ComputationalCryoEM/ASPIRE-Python/actions/runs/9565814400/job/26369634117#step:5:273

I'm going to take a quick look at it before I'm comfortable marking this ready.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Jun 20, 2024

And it failed again. There's whitening involved here. Might just need to adjust the atol here.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Jun 20, 2024

Added some np.testing to get a better idea of what we're dealing with here on the failing tests. Doesn't look like the failures are related to the changes made in this PR.

@garrettwrong
Copy link
Collaborator

garrettwrong commented Jun 20, 2024

@j-c-c , I'll run develop again now.

@garrettwrong
Copy link
Collaborator

garrettwrong commented Jun 20, 2024 •
edited
Loading

@j-c-c , I'll run develop again now.

Can follow here:
https://github.com/ComputationalCryoEM/ASPIRE-Python/actions/runs/9599598436

If it fails, I can look at the env from 3 days ago and see if anything updated.

@garrettwrong
Copy link
Collaborator

garrettwrong commented Jun 20, 2024

@j-c-c , I'll run develop again now.

Can follow here: https://github.com/ComputationalCryoEM/ASPIRE-Python/actions/runs/9599598436

If it fails, I can look at the env from 3 days ago and see if anything updated.

@j-c-c , I'll run develop again now.

Can follow here: https://github.com/ComputationalCryoEM/ASPIRE-Python/actions/runs/9599598436

If it fails, I can look at the env from 3 days ago and see if anything updated.

It did not fail, so something about this PR is likely causing this behavior. Have you tried reverting that interp cast change?

@j-c-c
Copy link
Collaborator Author

j-c-c commented Jun 20, 2024

Ok @garrettwrong, looks like reverting to using the upcast worked. Everything should be good to go once CI passes.

@j-c-c j-c-c force-pushed the whiten_threshold branch 2 times, most recently from f837800 to c2a1961 Compare June 24, 2024 17:42
@garrettwrong
Copy link
Collaborator

garrettwrong commented Jun 24, 2024

After discussion with Josh, this is going to get pushed to next release out of caution (and time constraints). The changes appear to be tickling some yet to be identified unstable code/bug. I'm not sure if it is this PR or something else, but the failures have not been reproduced outside this PR yet despite many runs.

@j-c-c j-c-c force-pushed the whiten_threshold branch from c2a1961 to cb9b42c Compare July 10, 2024 17:47
@j-c-c
Copy link
Collaborator Author

j-c-c commented Jul 10, 2024

I removed the changes made to the ArrayFilter involving the upcast/recast for scipy's interpolator. Hopefully this should resolve the CI issues we were seeing on the arm platform.

I left the dtype pass-through test that I wrote for this, but xfailed for singles. I also left in some conversions to np.testing in a few of the tests I was troubleshooting relating to the CI issues.

So ultimately, this PR contains only the addition of an epsilon argument used for configuring the whitening_filter and PowerFilter safeguard against dividing by small values.

garrettwrong
garrettwrong requested changes Jul 11, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick change, then will approve, thanks.

garrettwrong
garrettwrong previously approved these changes Jul 12, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@j-c-c j-c-c marked this pull request as ready for review July 12, 2024 13:22
@j-c-c j-c-c requested a review from janden as a code owner July 12, 2024 13:22
@j-c-c
Copy link
Collaborator Author

j-c-c commented Jul 12, 2024

@garrettwrong apparently the strict parameter is only for the xfail decorator. Removing

@garrettwrong
Copy link
Collaborator

garrettwrong commented Jul 12, 2024

I put it back and updated the branch. Just needed to use a slightly different syntax. Should be good to go.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Jul 12, 2024

I put it back and updated the branch. Just needed to use a slightly different syntax. Should be good to go.

Ah, thank you!

@j-c-c j-c-c force-pushed the whiten_threshold branch from 97bcede to 5167df1 Compare July 16, 2024 12:31
janden
janden requested changes Jul 22, 2024
View reviewed changes
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one thing.

@j-c-c j-c-c force-pushed the whiten_threshold branch from 5c0b352 to debeddc Compare July 23, 2024 16:45
j-c-c added 21 commits July 23, 2024 14:15
@j-c-c
Add epsilon arg to PowerFilter.
daf56ae
@j-c-c
Add threshold to whiten function with matlab default.
391005d
@j-c-c
Recast ArrayFilter result after scipy workaround upcast occurs.
7990997
@j-c-c
Revert to original threshold of eps(dtype) on PSD.
d028e1f
@j-c-c
remove comment
c6810ff
@j-c-c
set default epsilon inside whiten function.
bf9abbc
@j-c-c
test PowerFilter argument
700c0f2
@j-c-c
smoke test for whiten epsilon param.
340130d
@j-c-c
bump scipy version. remove upcast.
75dfe87
@j-c-c
test that whiten safeguard is actually working.
8f59c5c
@j-c-c
use np.testing in suite that failed on arm.
2b4b1d9
@j-c-c
use np.testing in test_FLEbasis2D.py
5a4dec3
@j-c-c
revert to upcasting.
240ba13
@j-c-c
update comments
9ded5a2
@j-c-c
remove scipy bump
3a1b0c9
@j-c-c
Revert scipy workaround.
f15cbfc
@j-c-c
xfail ArrayFilter test for singles.
9c6f90d
@j-c-c
make xfail strict
c244b42
@j-c-c
remove strict param
e397c31
@j-c-c
Use pytest fixtures.
622fcde
@j-c-c
remove unused import
8e02be8
@j-c-c j-c-c force-pushed the whiten_threshold branch from debeddc to 8e02be8 Compare July 23, 2024 18:15
@j-c-c j-c-c enabled auto-merge (rebase) July 23, 2024 18:38
@j-c-c j-c-c merged commit 2368196 into develop Jul 23, 2024
@garrettwrong garrettwrong mentioned this pull request Oct 8, 2024
Closed
@garrettwrong garrettwrong deleted the whiten_threshold branch November 20, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janden janden janden approved these changes

@garrettwrong garrettwrong garrettwrong left review comments

Assignees

@j-c-c j-c-c

Labels

cleanup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@j-c-c @garrettwrong @janden