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

Add test priors 2 #695

Merged
merged 8 commits into from
Dec 19, 2020
Merged

Add test priors 2 #695

merged 8 commits into from
Dec 19, 2020

Conversation

robbietuk
Copy link
Collaborator

@robbietuk robbietuk commented Sep 24, 2020

This PR is to replace and expand upon that of #519.

Addresses #153

@robbietuk
Copy link
Collaborator Author

robbietuk commented Sep 24, 2020

Travis will fail on PLS because of edge artefacts. Here is a difference image. PLS uses a uniform 1's anatomical image and the same randomised densitity (between 0 and 1 voxel values) as QP and RDP
image

Codacy is still not happy for the same reasons as mentioned #519 (comment)

@robbietuk
Copy link
Collaborator Author

robbietuk commented Sep 24, 2020

I believe PLS is failing at the (max) edge because of continue statements in the directional gradient computation

if(direction==0){
if(z+1>max_z)
continue;
image_gradient_elem[z][y][x]=image[z+1][y][x]- image[z][y][x];
}
if(direction==1){
if(y+1>max_y)
continue;
image_gradient_elem[z][y][x]=image[z][y+1][x]- image[z][y][x];
// std::cout<<"grady ="<<image[z][y+1][x]- image[z][y][x]<<std::endl;
}
if(direction==2){
if(x+1>max_x )
continue;
image_gradient_elem[z][y][x]=image[z][y][x+1]- image[z][y][x];

The solution to this is not obvious.

The failure at the other edge (top and left) is harder to reason but I have seen similar issues with boundary conditions with RDP and QP. I have not had a chance to look through the PLS code.

@robbietuk robbietuk mentioned this pull request Nov 17, 2020
@KrisThielemans
Copy link
Collaborator

@robbietuk, what about disabling the PLS test for now, adding logcosh and merging this PR?

@robbietuk
Copy link
Collaborator Author

Yes, I think we also expand the testing for RDP and logcosh priors to have more than one evaluation with different gamma(RDP)/scalar(logcosh) values. We didn't spot the logcosh error regarding the value compuation when the input is large for a couple of weeks.

Ill get on this shortly, did you want it for release4.1?

@robbietuk
Copy link
Collaborator Author

robbietuk commented Dec 18, 2020

test_priors passes on my machine (waiting on Travis to confirm). This PR needs to be followed up with a further issue relating to additional tests for other prior methods, but these should wait until the mess with the priors code has been cleaned up (#694). The tests should also be expanded to vary paramerter for other methods.

@KrisThielemans KrisThielemans merged commit f5025b2 into UCL:master Dec 19, 2020
@robbietuk robbietuk deleted the add_test_priors branch January 2, 2021 13:12
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.

2 participants