Skip to content

ENH: add average error to ErrorMap #1039

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

Merged
merged 9 commits into from
Feb 20, 2015

Conversation

lsqshr
Copy link
Contributor

@lsqshr lsqshr commented Feb 2, 2015

Add an average distance output to the nipype.algorithms.metrics.ErrorMap, which simpy get the square root average/the sum average of the errormap vector.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.28% when pulling ba11e7c on lsqshr:enh_add_average_error_2_errormap into 66ba90b on nipy:master.

@satra
Copy link
Member

satra commented Feb 4, 2015

@oesteban - could you please take a look at this? looks fine to me.

@lsqshr - could you please add an updated CHANGES file?

also it would be good add a test for this that uses nibabel to generate some fixed, known data and evaluates the metrics.

@lsqshr
Copy link
Contributor Author

lsqshr commented Feb 5, 2015

@satra Thanks for the comments. I will update the changes file and make some according tests.

@lsqshr
Copy link
Contributor Author

lsqshr commented Feb 13, 2015

@oesteban - in the following chunk of code in ErrorMap, were you just trying to calculate the euclidean distance voxel-wisely (np.abs(diffvector))? Or it was just intended to calculate a norm of two images?

diffvector = (refvector-tstvector)
...
...
elif self.inputs.metric == 'euclidean':
            X = np.hstack((refvector, tstvector))
            errvector = np.linalg.norm(X, axis=1)

double checking this because I am making some tests for it -- Thanks in advance

@oesteban
Copy link
Contributor

@lsqshr you are right, but if I recall it well, I use np.linalg.norm for the case that refvector and tstvector are multispectral (meaning, they have more than one component per pixel).

The np.hstack is superfluous though, as you say, this can be simplified to errvector = np.linalg.norm(diffvector, axis=1)

@satra
Copy link
Member

satra commented Feb 13, 2015

perhaps we can also add the frobenius norm here and keep expanding the list of metrics.

@lsqshr
Copy link
Contributor Author

lsqshr commented Feb 16, 2015

  • @oesteban in this case I would change np.hstack to diffvector in this PR.
  • @satra cool, I will add the frobenius norm in this PR for now

…euclidean average

`errvectorexp = np.zeros_like(mskvector)` used to generate a uint8
vector by default which loses the precision when calculating the special
volumes with float voxels of small values like the jacobian determinant
volume
@lsqshr lsqshr changed the title WIP: add average error to ErrorMap ENH: add average error to ErrorMap Feb 19, 2015
@lsqshr
Copy link
Contributor Author

lsqshr commented Feb 19, 2015

Just made the update and some tests with dummy 2*2*2 volumes

mskvector = msk.reshape(-1)
msk_idxs = np.where(mskvector==1)
refvector = ref_data.reshape(-1,comps)[msk_idxs].astype(np.float32)
tstvector = tst_data.reshape(-1,comps)[msk_idxs].astype(np.float32)
diffvector = (refvector-tstvector)

# Scale the diffrernce
Copy link
Contributor

Choose a reason for hiding this comment

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

Minimal typo (should read difference).

@oesteban
Copy link
Contributor

@lsqshr the new code looks great, I've added some very minor comments. Additionally, we would need you to update your branch before I can merge the PR (step 1, step 2). If you need more help with that, do not hesitate to write me :).

@lsqshr
Copy link
Contributor Author

lsqshr commented Feb 20, 2015

@oesteban, thanks for reviewing the code and the comments. I have just made the according fixes and synced this branch with the upstream.

oesteban added a commit that referenced this pull request Feb 20, 2015
@oesteban oesteban merged commit 453bcf9 into nipy:master Feb 20, 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.

4 participants