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

[FIX] testing line-based target function #1684

Merged
merged 2 commits into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@skoudoro
Copy link
Member

skoudoro commented Dec 8, 2018

The aim of this PR is to fix #1679.

line-based target function was failing for some random case so I replaced np.randomby a static array.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 8, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@d0de4d3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1684   +/-   ##
=========================================
  Coverage          ?   84.25%           
=========================================
  Files             ?      114           
  Lines             ?    13620           
  Branches          ?     2141           
=========================================
  Hits              ?    11475           
  Misses            ?     1648           
  Partials          ?      497
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 8, 2018

Do we understand why the test is failing?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 8, 2018

If the conditions under which this test fails are realistic, then it is a bug that needs to be fixed. If they are unrealistic, it should be detected and an informative error should be raised.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 8, 2018

They are realistic. However, I do not think it give us a wrong result or there is a bug.

The input streamlines are close to each other and there is some crossing so I am not surprised that some transformations/deformations put 2 streamlines in the mask instead of 1.

I might miss something but I am surprised that this test was passing all the time until now.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 9, 2018

I thought that passing the affine as input to the function should undo any transformation/deformation induced by the affine.

random_array = np.array([[0.2862315, 0.44142904, 0.19837613, 0.422711],
[0.7434331, 0.55335599, 0.18633461, 0.427429],
[0.1465992, 0.84593179, 0.79033433, 0.576709],
[0.6525159, 0.96735703, 0.69833409, 0.117800]])

This comment has been minimized.

@arokem

arokem Dec 9, 2018

Member

If you're going to hard code this, you can set the last row to [0, 0, 0, 1] and subtract 0.5 upfront?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 9, 2018

ok, so I will play more with this function to make sure:

  • I do not misunderstand it
  • There is no bug

Thank you for the feedback @arokem

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 10, 2018

Hi @skoudoro, @arokem and @jchoude. The affine used in the test should not be a random matrix which is what it was before. A random matrix is not necessarily an affine matrix even with the 0001 correction of the last row. Serge you can go ahead and use a more reasonable matrix i.e.

2 0 0 0
0 1 0 0 
0 0 1 0
0 0 0 1

and then @jchoude can provide a more advanced version of that at his convenience. Cheers!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 10, 2018

That makes sense. What are the conditions for a matrix to be affine? Needs to be invertible, what else?

I believe that the matrix you provided would not do what is intended here, which IIUC was to check what happens with smaller voxels. IIUC, you want diagonal elements that are <1.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 10, 2018

Ariel, that was only a suggestion. Be happy to change this to a new version such as

.5 0 0 0
0 .5 0 0 
0 0 .4 0
0 0 0 1
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 10, 2018

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 10, 2018

+1 link looks solid!

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 10, 2018

If want to generate random affines...

A nice utility function, it deserves its PR. 😄

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 11, 2018

I just created #1687 as a reminder.

I think this PR is ready to go!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 11, 2018

Yup. Off we go.

@arokem arokem merged commit bec8163 into nipy:master Dec 11, 2018

4 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:issue-1679-target-lb branch Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment