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
base: master
from

Conversation

Projects
None yet
4 participants
@matthew-brett
Member

matthew-brett commented Aug 1, 2015

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?

WIP: ensure inverse affine is set
Set `affine_inv` unconditionally to make sure that it is set to None,
even if the affine is not valid.
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 2, 2015

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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Aug 12, 2015

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

This comment has been minimized.

Member

Garyfallidis commented Aug 12, 2015

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

This comment has been minimized.

Member

matthew-brett commented Aug 12, 2015

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

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 12, 2015

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

This comment has been minimized.

Member

Garyfallidis commented Aug 12, 2015

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

This comment has been minimized.

Contributor

omarocegueda commented Aug 12, 2015

Hi @matthew-brett @Garyfallidis,

can you make a PR asap with tests for the AffineMap

sure!, this will be my priority today.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Aug 13, 2015

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

This comment has been minimized.

Member

arokem commented Oct 2, 2015

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

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 2, 2015

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