Skip to content

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Apr 3, 2014

Created a new nipype.algorithms.eval module for methods performing
error, distance or similitude computations over images.

I think this is better than having them mixed up with, e.g.,
nifti or csv files management.

I also add a new interface: ErrorMap, that computes a distance
function between two images (e.g. two displacement fields). Several
metrics will be available, however now I contribute only with
squared differences maps.

oesteban added 4 commits April 3, 2014 14:59
Created a new nipype.algorithms.eval group for methods performing
simple error, distance or similitude computations over images.

I think this is better than having them mixed up with, e.g.,
nifti or csv files management.

I also add a new interface: ErrorMap, that computes a distance
function between two images (e.g. two displacement fields). Several
metrics will be available, however now I contribute only with
squared differences maps.
@oesteban oesteban changed the title Evaluation altorithms separated from misc Evaluation algorithms separated from misc Apr 3, 2014
@oesteban
Copy link
Contributor Author

oesteban commented Apr 6, 2014

Tests are failing because they were not migrated (to-do)

@chrisgorgo
Copy link
Member

Thanks, could you fix the tests? Is the API backwards compatible?

@oesteban
Copy link
Contributor Author

Sure, I'll fix the tests ASAP.

Regarding compatibility, the interfaces moved from misc are exactly as they were at master. I just add this ErrorMap interface, which works pretty similarly to the existing ones.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 87d95cb on oesteban:enh/EvaluationAlgorithms into cba8e41 on nipy:master.

@oesteban
Copy link
Contributor Author

@chrisfilo I think I fixed the tests, however travis does not pass for python 2.6 (I don't know why).

@chrisgorgo
Copy link
Member

The 2.6 failure is unrelated. Two more things, mention the new interface in CHANGES, cover the case when user wants to import nipype.algorithms.misc.Distance with an deprecated warning.

Thanks in advance!

@@ -28,6 +28,10 @@

from .. import logging

from .. import warnings
warnings.warn('Evaluation interfaces (Distance, Overlap, FuzzyOverlap)
have been moved to nipype.algorithms.eval' )
Copy link
Member

Choose a reason for hiding this comment

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

This will be shown each time someone imports anything from misc. Would there be a way for people to still be able to import (Distance, Overlap, FuzzyOverlap) from misc but get a warning when they do that, but not get a warning when they import anything else?

Copy link
Member

Choose a reason for hiding this comment

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

also could we maintain backward compatibility for one release (@chrisfilo this should be 0.10 not 1.0 yet) and then deprecate the following release. this warning should change to a DeprecationWarning and import these interfaces from .eval import ...

if we really wanted to give the warning only when someone imports from misc, we would have to add dummy classes, something like:

(also could rename eval to metrics?)

import nipype.algorithms.metrics as nam

class Distance (nam.Distance):
    DeprecationWarning(...)

Copy link
Member

Choose a reason for hiding this comment

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

+1 for backward compatibility!

On Tue, May 27, 2014 at 1:36 PM, Satrajit Ghosh notifications@github.comwrote:

In nipype/algorithms/misc.py:

@@ -28,6 +28,10 @@

from .. import logging

+from .. import warnings
+warnings.warn('Evaluation interfaces (Distance, Overlap, FuzzyOverlap)

  •           have been moved to nipype.algorithms.eval' )
    

also could we maintain backward compatibility for one release (@chrisfilohttps://github.com/chrisfilothis should be 0.10 not 1.0 yet) and then deprecate the following release.
this warning should change to a DeprecationWarning and import these
interfaces from .eval import ...

if we really wanted to give the warning only when someone imports from
misc, we would have to add dummy classes, something like:

(also could rename eval to metrics?)

import nipype.algorithms.metrics as nam

class Distance (nam.Distance):
DeprecationWarning(...)


Reply to this email directly or view it on GitHubhttps://github.com//pull/830/files#r13072770
.

@oesteban
Copy link
Contributor Author

Ok, no problem. I will update ASAP

@oesteban
Copy link
Contributor Author

Ok, it's backwards compatible now. Eval renamed to metrics.

I've included the warnings in the constructor, so they are called only when you are actually using the interface.

The warnings are DeprecationWarnings, so they are ignored by default (just to mention it).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) when pulling 13aee60 on oesteban:enh/EvaluationAlgorithms into ab0e8b9 on nipy:master.

Also fixed constructor of old interface to throw the DeprecationWarning
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling 7c8846f on oesteban:enh/EvaluationAlgorithms into ab0e8b9 on nipy:master.

Additionally, docstrings have been improved for several methods,
especially indicating deprecated classes.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling 960a374 on oesteban:enh/EvaluationAlgorithms into ab0e8b9 on nipy:master.

Now it uses numpy.linalg.norm
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) when pulling 664b93c on oesteban:enh/EvaluationAlgorithms into ab0e8b9 on nipy:master.

chrisgorgo added a commit that referenced this pull request Jul 24, 2014
Evaluation algorithms separated from misc
@chrisgorgo chrisgorgo merged commit d8fd8dd into nipy:master Jul 24, 2014
@oesteban oesteban deleted the enh/EvaluationAlgorithms branch July 24, 2014 10:34
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