Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Cleaned up a little so we no longer crash with div0 or singular matrices. However, estimated result for clean case get_cwf_coeffs doesn't seem right (to me).

Maybe its too degenerate a case and my expectations are wrong? If that is the case we can document, log, then no-op or something.. Essentially I'm hoping to be able to setup some code and turn noise on/off without effectively writing a totally new experiment... We might need that for say integrating with class averaging or other developmental codes using clean images to start with...

Closes #420

@garrettwrong garrettwrong added bug Something isn't working documentation Improvements or additions to documentation cleanup labels May 25, 2021
@garrettwrong garrettwrong self-assigned this May 25, 2021
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #421 (cbb27f6) into develop (004ba5e) will increase coverage by 0.79%.
The diff coverage is 95.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #421      +/-   ##
===========================================
+ Coverage    86.93%   87.73%   +0.79%     
===========================================
  Files           91       95       +4     
  Lines         6171     6630     +459     
===========================================
+ Hits          5365     5817     +452     
- Misses         806      813       +7     
Impacted Files Coverage Δ
src/aspire/covariance/covar2d.py 93.94% <95.45%> (+1.39%) ⬆️
src/aspire/commands/_config.py 0.00% <0.00%> (ø)
src/aspire/ctf/ctf_estimator.py 98.87% <0.00%> (ø)
src/aspire/commands/estimate_ctf.py 0.00% <0.00%> (ø)
src/aspire/ctf/__init__.py 100.00% <0.00%> (ø)
src/aspire/utils/misc.py 100.00% <0.00%> (+2.77%) ⬆️
src/aspire/image/image.py 92.88% <0.00%> (+4.05%) ⬆️
src/aspire/utils/coor_trans.py 96.11% <0.00%> (+7.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 004ba5e...cbb27f6. Read the comment docs.

@garrettwrong
Copy link
Collaborator Author

I think the batched issue was because may be because were on the edge of tolerance in singles. Probably the larger magnitudes (dividing by small noise_var) helped before. It's possible there is more to it though...

The tutorial result, while still passable, appears to have more error now. I was expecting it to be tail noise, but its noticeable.

For noise_var=0 I think its still not right (even with get_cwf_coeffs returning the coeffs identically now). I get closer with negation, and I see a curious negative sign in the tutorial plots. Maybe its superfluous, but do you recall why that would be there?

@janden
Copy link
Collaborator

janden commented May 26, 2021

I think the batched issue was because may be because were on the edge of tolerance in singles. Probably the larger magnitudes (dividing by small noise_var) helped before. It's possible there is more to it though...

Ah ok. I assumed it had something to do with the new default noise_var. It was using noise_var = 1 before, which shouldn't have resulted in large magnitudes.

The tutorial result, while still passable, appears to have more error now. I was expecting it to be tail noise, but its noticeable.

That's strange. Are you talking about with non-zero noise_var? It could be that I messed up the formulas for the shrinkage (although the relevant tests pass, so that shouldn't happen). What happens if you try without shrinkage?

For noise_var=0 I think its still not right (even with get_cwf_coeffs returning the coeffs identically now). I get closer with negation, and I see a curious negative sign in the tutorial plots. Maybe its superfluous, but do you recall why that would be there?

There's bound to be some residual error due to projection onto the basis (I see some rings outside of the central disk), but otherwise it should be similar, especially if get_cwf_coeffs is set to pass through for noise_var = 0, since then it's just a matter of evaluate_t followed by evaluate.

@garrettwrong
Copy link
Collaborator Author

shrinker=None with the stock noise_var in the cov2d tutorial is closer (probably okay). Closer than frobenius_norm.

Can't compare the shrinker=None with noise_var=0 because that case was previously not working.

I think in the tutorial I am seeing other things, maybe its okay. Can talk about it tomorrow. I'll try to have them ready to run and maybe can close this out then.

@garrettwrong garrettwrong changed the title fix None vs "None" and 0 vs 1 Fixup covar2d shrinker and get_cwf_coeffs for noise_var=0 May 28, 2021
@garrettwrong garrettwrong marked this pull request as ready for review May 28, 2021 19:59
@garrettwrong garrettwrong requested a review from janden as a code owner May 28, 2021 19:59
@garrettwrong garrettwrong linked an issue May 28, 2021 that may be closed by this pull request
5 tasks
@garrettwrong garrettwrong merged commit 0fd9e35 into develop Jun 1, 2021
@garrettwrong garrettwrong deleted the fix_covar0 branch June 8, 2021 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cov2d noise_var=0 appears to have a bug (crash)

3 participants