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

WIP: affine map tests #693

Closed
wants to merge 1 commit into from

Conversation

matthew-brett
Copy link
Contributor

Here I did the change to set_affine I mentioned in the PR discussion
#690 (comment)

I was going to add a test, but then I noticed that AffineMap has no tests -
can that be right?

Set `affine_inv` unconditionally to make sure that it is set to None,
even if the affine is not valid.
@Garyfallidis
Copy link
Contributor

Probably, because the AffineMap was to be discussed in a following PR and first see if it can be used with SLR.

@matthew-brett
Copy link
Contributor Author

So was the plan to leave AffineMap in without tests until then? Can you be say more about what this future PR might be?

@Garyfallidis
Copy link
Contributor

Hi @matthew-brett ! Thanks for the reminder. No, this is definitely not staying in without tests. The first question should be if we need the AffineMap. The AffineMap is currently not being used for registration. The tutorial is misleading!

The PR should be about the AffineMap and it's possibility to be used with SLR. Maybe two PRs in total. One from me and one from Omar. Mine will probably come later. There was also some discussion about separating the linear metrics with the nonlinear. I would like to see a PR for that too.

On a side note I am currently running both the affine registration and the nonlinear registration with some demanding data. All good so far.

@matthew-brett
Copy link
Contributor Author

It looks like the AffineMap is being used in MutualInformationMetric. Not so?

@Garyfallidis
Copy link
Contributor

I was talking about this tutorial https://github.com/nipy/dipy/blob/master/doc/examples/affine_registration_3d.py#L50
The AffineMap is used there but not really used for registration.

@Garyfallidis
Copy link
Contributor

Oh damn. I missed this. He has used it in the actual imaffine file. No okay I understand now why you were worried about this. @omarocegueda can you make a PR asap with tests for the AffineMap?

@omarocegueda
Copy link
Contributor

Hi @matthew-brett @Garyfallidis,

can you make a PR asap with tests for the AffineMap

sure!, this will be my priority today.

@omarocegueda
Copy link
Contributor

Hi @matthew-brett , @Garyfallidis,
I just added the missing tests for AffineMap in PR #700 (I included this PR's commit there too). This is the reported test coverage (97%):
https://travis-ci.org/nipy/dipy/jobs/75506207
Thanks!

@arokem
Copy link
Contributor

arokem commented Oct 2, 2015

Is this superseded by #700? Should we close this one?

@omarocegueda
Copy link
Contributor

Is this superseded by #700? Should we close this one?

Yes, #700 includes Matthew's change.

@arokem arokem closed this Oct 2, 2015
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.

None yet

4 participants