fix: use matlab_cmd provided as input closes #2442 by satra · Pull Request #2452 · nipy/nipype

satra · 2018-02-19T20:40:27Z

see discussion in #2442

Closes #2436.

codecov-io · 2018-02-19T23:05:50Z

Codecov Report

Merging #2452 into master will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
+ Coverage   66.62%   66.65%   +0.02%     
==========================================
  Files         328      328              
  Lines       42508    42513       +5     
  Branches     5280     5277       -3     
==========================================
+ Hits        28321    28337      +16     
+ Misses      13505    13496       -9     
+ Partials      682      680       -2

Flag	Coverage Δ
#smoketests	`50.74% <57.14%> (+0.01%)`	⬆️
#unittests	`63.97% <92.85%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/spm/base.py	`86.75% <92.85%> (+2.91%)`	⬆️
nipype/interfaces/spm/preprocess.py	`57.51% <0%> (+0.23%)`	⬆️
nipype/workflows/fmri/spm/preprocess.py	`72.44% <0%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04d0159...88dbf6e. Read the comment docs.

…rsion info

satra · 2018-02-20T23:27:25Z

@djarecka and @effigies - would appreciate a review here.

effigies · 2018-02-21T00:07:15Z

+    If you use `SPMCommand.set_mlab_paths` to set alternate entries for
+    matlab_cmd, paths, and use_mcr, then you will need to use the same entries
+    to any call in the Info class to maintain memoization. Otherwise, it will
+    default to the parameters in the `getinfo` function below.


I'm conceptually a little displeased with this memoization principle, but in practice I imagine nobody's using multiple MATLABs and SPMs within a single nipype script, so it's probably fine.

actually i think this exactly handles the multiple matlab/spm scenario. let's say i have two different versions of spm. i can explicitly use Info to retrieve them (yes, it will loose the first memory on collecting the second one).

the situation, i think, boils down to this.

SPMCommand.set_mlab_paths is a class method, so every instance derived will respect it. and i can adjust any given instance to change its command/path.

Info only has class methods and we would somehow need to detect a change. this method decouples the change detection saying the only memory it has is when you pass the same parameters.

an alternative was the previous augmented version to set the Info attributes everytime set_mlab_paths is called, retrieve that information and store it and always return that unless its changed again with get_info call.

i don't feel strongly about either. just think that the latter seems more magical.

Agreed, in the sense that this will be correct. The issue I was seeing with multiple versions I have is that you're running a MATLAB instance every time the cache is invalidated to get the information.

But it really doesn't matter. If people start complaining about slowdowns, we can consider keeping a memoizing dict keyed on a 3-tuple. This is probably fine, though.

effigies · 2018-02-21T01:33:09Z

Merged master to bump the tests.

djarecka · 2018-02-21T01:49:06Z

    _path = None
    _name = None
+    _command = None
+    _paths = None


shouldn't you also add _version = None

* upstream/master: CI: Rebalance Circle jobs DOCKER: Hard-code Miniconda version CI: Revert to Miniconda 4.3.31 FIX: asn1crypto 0.24 incompatible with Python <= 3.6 drop Upcoming release if present add URL, drop Upcoming release Fix my doctest and add test_auto file Fix doctest and make vars more explicit Fix doctest filenames remove -e which should not be there [ENH] Automate updates of CHANGES Add first remarks from PR Add SPM Fieldmap Tool wrapper Added: - VDM Calculation pre-substracted phase case (SIEMENS machines) Todo: - Support VDM Application - Support different magnitude and phase file types

* origin/fix/spminfo:

djarecka · 2018-02-21T02:07:54Z

@satra this is just a general comment, not related to your changes, but possibly you can add some comments to the docstrings so it's clear what is expected as paths (e.g. here). It was a bit confusing for me where matlab command/path is needed and where spm path is required.

djarecka · 2018-02-21T02:16:48Z

+                      or os.getenv('MATLABCMD', 'matlab -nodesktop -nosplash'))
+
+        if klass._name and klass._path and klass._version and \
+                klass._command == matlab_cmd and klass._paths == paths:


This is related to your discussion with @effigies. Shouldn't we add some warnings if klass._command != matlab_cmd or klass._paths != paths. Not sure how likely this might eb due to mistake.

i'm not a big fan of warnings. almost no one reads it.

well, you're probably right, but personally I sometimes read them if I feel hopeless...

satra · 2018-02-23T01:15:31Z

is this ok for merge?

effigies · 2018-02-23T01:17:44Z

@djarecka is testing on her machine right now. It looked like she had a couple questions. Are you comfortable with the logic, given those questions?

@jykim24 I believe this PR derives from your #2436 issue. Are you able to check whether this branch resolves your issue?

satra · 2018-02-23T01:26:38Z

@effigies - sorry had forgotten to commit and push.

djarecka · 2018-02-23T01:39:23Z

i've never used Matlab on my OSX and had to figure out the license etc., but now it looks like it can find a version of spm (but Info.name() returns None):

>>> from nipype.interfaces import spm
>>> spm.Info.name()
>>> spm.SPMCommand().version
>>> spm.SPMCommand.set_mlab_paths(paths="/Users/dorota/spm12", matlab_cmd="/Applications/MATLAB_R2014b.app/bin/matlab -nodesktop -nosplash")
>>> spm.Info.name()
>>> spm.SPMCommand().version
'12.7219'

satra · 2018-02-23T02:25:24Z

@djarecka - your output is correct. to get the same version as returned by som.SPMCommand() you would have to call Info.name() with the same parameters.

i could make the change that Info always follows what SPMCommand sets, but this would require a change of API to some extent. i wouldn't want name to have parameters and get_info with the appropriate parameters would always need to be called.

in the version before the current change: Info.name() would always return a fixed value once set.

let me think about this and determine which i like more tomorrow.

djarecka · 2018-02-23T02:30:56Z

oh yes, I completely forgot! Now everything looks good:

>>> from nipype.interfaces import spm
>>> spm.Info.name()
>>> spm.SPMCommand().version
>>> spm.SPMCommand.set_mlab_paths(paths="/Users/dorota/spm12", matlab_cmd="/Applications/MATLAB_R2014b.app/bin/matlab -nodesktop -nosplash")
>>> spm.SPMCommand().version
'12.7219'
>>> spm.Info.name(paths="/Users/dorota/spm12",  matlab_cmd="/Applications/MATLAB_R2014b.app/bin/matlab -nodesktop -nosplash")
'SPM12'

djarecka · 2018-02-23T02:42:15Z

            If none of the above was successful, the fallback value of
            'matlab -nodesktop -nosplash' will be used.
        paths : str
+            Add paths to matlab session


shouldn't we suggest that here should be also path to spm (that can be completely different than matlab) if we want to run spm interfaces ?

IMO, that's clear from the function description: "Returns the path to the SPM directory in the Matlab path".

effigies · 2018-02-23T15:12:06Z

Updated the description to auto-close #2436. Undo if this only partially resolves that issue.

fix: use matlab_cmd provided as input closes nipy#2442

b44bce4

satra added 2 commits February 20, 2018 12:07

fix: ensure that changes to matlab paths and commands generate new ve…

94b316d

…rsion info

fix: change the way memoization works for the Info class

44a7b66

effigies reviewed Feb 21, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/spminfo

94c926b

djarecka reviewed Feb 21, 2018

View reviewed changes

satra added 3 commits February 20, 2018 20:54

Merge remote-tracking branch 'origin/fix/spminfo' into fix/spminfo

328398d

* origin/fix/spminfo:

fix: add missing version

1572498

djarecka reviewed Feb 21, 2018

View reviewed changes

effigies added this to the 1.0.1 milestone Feb 23, 2018

fix: address djarecka's questions

88dbf6e

djarecka reviewed Feb 23, 2018

View reviewed changes

effigies approved these changes Feb 23, 2018

View reviewed changes

effigies merged commit a1363b4 into nipy:master Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: use matlab_cmd provided as input closes #2442 - #2452

fix: use matlab_cmd provided as input closes #2442#2452
effigies merged 8 commits into
nipy:masterfrom
satra:fix/spminfo

satra commented Feb 19, 2018 •

edited by effigies

Loading

codecov-io commented Feb 19, 2018 •

edited

Loading

satra commented Feb 20, 2018

effigies Feb 21, 2018

satra Feb 21, 2018

effigies Feb 21, 2018

effigies commented Feb 21, 2018

djarecka Feb 21, 2018

djarecka commented Feb 21, 2018

djarecka Feb 21, 2018

satra Feb 23, 2018

djarecka Feb 23, 2018

satra commented Feb 23, 2018

effigies commented Feb 23, 2018

satra commented Feb 23, 2018

djarecka commented Feb 23, 2018

satra commented Feb 23, 2018

djarecka commented Feb 23, 2018

djarecka Feb 23, 2018

effigies Feb 23, 2018

effigies commented Feb 23, 2018

Labels

4 participants

Uh oh!

Conversation

satra commented Feb 19, 2018 • edited by effigies Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov-io commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

satra commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Feb 21, 2018

Choose a reason for hiding this comment

djarecka commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satra commented Feb 23, 2018

effigies commented Feb 23, 2018

satra commented Feb 23, 2018

djarecka commented Feb 23, 2018

satra commented Feb 23, 2018

djarecka commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Feb 23, 2018

Labels

4 participants

satra commented Feb 19, 2018 •

edited by effigies

Loading

codecov-io commented Feb 19, 2018 •

edited

Loading