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

Small correction on KFA #2405

Merged
merged 3 commits into from Jun 18, 2021
Merged

Small correction on KFA #2405

merged 3 commits into from Jun 18, 2021

Conversation

RafaelNH
Copy link
Contributor

@RafaelNH RafaelNH commented Jun 1, 2021

This should be a small correction on KFA definition - it has to be zero if the kurtosis tensor is null.

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #2405 (1094a97) into master (541b5b2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2405      +/-   ##
==========================================
- Coverage   85.26%   85.25%   -0.02%     
==========================================
  Files         125      125              
  Lines       16581    16581              
  Branches     2686     2685       -1     
==========================================
- Hits        14138    14136       -2     
  Misses       1759     1759              
- Partials      684      686       +2     
Impacted Files Coverage Δ
dipy/reconst/dki.py 96.37% <100.00%> (+0.01%) ⬆️
dipy/reconst/forecast.py 89.84% <0.00%> (-0.51%) ⬇️
dipy/viz/app.py 77.01% <0.00%> (-0.49%) ⬇️
dipy/core/gradients.py 91.03% <0.00%> (+0.28%) ⬆️

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @RafaelNH,

Thank you for the update. it seems your test is failing on our CI's:

>       assert_almost_equal(dkiF.kfa, 0)
E       AssertionError: 
E       Arrays are not almost equal to 7 decimals
E       
E       Mismatched elements: 1 / 1 (100%)
E       Max absolute difference: 0.8582412541188063
E       Max relative difference: inf
E        x: array(0.8582412541188063)
E        y: array(0)

more information here

Can you have a quick look? Thanks!

@skoudoro skoudoro added this to the 1.5.0 milestone Jun 1, 2021
@skoudoro
Copy link
Member

skoudoro commented Jun 7, 2021

Hi @RafaelNH,

this a just a small ping concerning my question/comment above.

@RafaelNH
Copy link
Contributor Author

Hi @skoudoro! Thanks for pointing that the test was failing! At least, it was ensuring that the added test detected that issue. This test failure was related to KFA estimates for null kurtosis zeros (in which MKT = 0). MKT was supposed to be zero on the added test, but due to numerical precision is was having a small magnitude. It didn't fail on my pc, because my pc computed a negative residual MKT values, while in the CI tests MKT was giving a small positive residual value. To fix the issue, I am now using a small tolerance to detect null kurtosis tensors (i.e. MKT < tol).

@arokem
Copy link
Contributor

arokem commented Jun 15, 2021

Looks like the remaining failures are the ones that will be fixed through #2407, so I think this is good to go.

@skoudoro
Copy link
Member

I agree, good to go.

Thank you @RafaelNH for this fix! merging

@skoudoro skoudoro merged commit 3901687 into dipy:master Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants