search

ComputationalCryoEM / ASPIRE-Python

Algorithms for Single Particle Reconstruction
http://spr.math.princeton.edu
GNU General Public License v3.0
46 stars 21 forks source link

Whitening Filter Threshold #1125

Closed j-c-c closed 2 months ago

j-c-c commented 3 months ago

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).

garrettwrong commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

j-c-c commented 3 months ago

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 commented 3 months ago

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

garrettwrong commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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

garrettwrong commented 3 months ago

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 commented 2 months ago

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.

j-c-c commented 2 months ago

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

garrettwrong commented 2 months ago

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

j-c-c commented 2 months ago

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

Ah, thank you!