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

[ENH] update for networkx dev version (should fix #2159) #2171

Closed
wants to merge 8 commits into from

Conversation

djarecka
Copy link
Collaborator

@djarecka djarecka commented Aug 27, 2017

The new code should work with both versions -- ver 1.x and 2.x (dev).

summary of changes:

  • *_iter has been removed, changing nodes_iter to nodes, etc. nodes gives dict-like NodeView (iterate or change to list first to access a specific element)

  • .node has been removed, changing to nodes[node]

  • add_node and add_edge don't have attr_dict anymore, have to change **dict

  • subgraph gives a view and .copy() has different depth, created two versions of this part . Suggestions how to simplify are welcome, don't understand all details of those various copies.

Note, I changed also interfaces.cmtk, but don't have any tests, so don't know if it works.

Edit (by @effigies):

Closes #2159
Closes #2194

@codecov-io
Copy link

codecov-io commented Aug 27, 2017

Codecov Report

Merging #2171 into master will decrease coverage by 0.04%.
The diff coverage is 42.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2171      +/-   ##
==========================================
- Coverage   72.28%   72.24%   -0.05%     
==========================================
  Files        1171     1171              
  Lines       58622    58626       +4     
  Branches     8433     8434       +1     
==========================================
- Hits        42377    42352      -25     
- Misses      14887    14918      +31     
+ Partials     1358     1356       -2
Flag Coverage Δ
#smoketests 72.24% <42.68%> (-0.05%) ⬇️
#unittests 69.9% <41.46%> (-0.05%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/plugins/tests/test_somaflow.py 50% <0%> (ø) ⬆️
nipype/interfaces/cmtk/nx.py 19.39% <0%> (ø) ⬆️
nipype/interfaces/cmtk/cmtk.py 18.77% <0%> (ø) ⬆️
nipype/interfaces/cmtk/nbs.py 36.55% <0%> (ø) ⬆️
nipype/pipeline/plugins/tests/test_oar.py 71.05% <0%> (ø) ⬆️
nipype/interfaces/cmtk/parcellation.py 14.46% <0%> (ø) ⬆️
nipype/pipeline/plugins/tests/test_pbs.py 71.79% <0%> (ø) ⬆️
nipype/info.py 84.61% <100%> (+0.24%) ⬆️
nipype/pipeline/engine/tests/test_engine.py 93.43% <100%> (ø) ⬆️
...pipeline/plugins/tests/test_multiproc_nondaemon.py 53.52% <100%> (ø) ⬆️
... and 7 more

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 4a0bd3d...6876a38. Read the comment docs.

@effigies
Copy link
Member

effigies commented Sep 21, 2017

At a glance, this looks reasonable to me, but I'm not as conversant with networkx as I'd like to be to sign off on this. We're starting to run up against this issue in our tests, so hopefully this can go in soon.

@effigies
Copy link
Member

@djarecka could you merge master? I can set fmriprep to running with this branch, which should at least smoke test most of these changes (and get our tests working agaiin).

…(*) in a few places (nodes_iter, in_edges_iter, etc., dont exist in ver 2, but nodes, in_edges, etc., are iterators); had to add .copy to .subgraph for ver 2 (otherwise its a view), but had to keep original code for ver 1
…more (so changed to **dict); some updates related to changes in .nodes method (its not a list anymore)
…they have no tests, so no idea if changes work
@djarecka
Copy link
Collaborator Author

@effigies - i'm trying quickly to check the error. It works fine on a few environments on my OSX and i noticed that python complains about ModuleNotFoundError: No module named 'pydot'. It doesn't complain for py3.4. Any thoughts?

@effigies
Copy link
Member

effigies commented Sep 22, 2017

Need to switch pydotplus in requirements and nipype/info.py to pydot.

networkx/networkx@481f3e8

Edit: Actually, here are all the instances of pydotplus: https://github.com/nipy/nipype/search?utf8=%E2%9C%93&q=pydotplus&type=

@djarecka
Copy link
Collaborator Author

@effigies thanks, i indeed installed pydot but completely forgot that it's not already in nipype... Still not sure, why it works for 3.4, but will update.

@effigies
Copy link
Member

Realizing that switching from pydotplus to pydot is going to break compatibility with networkx < 2. @satra is this okay (in which case we should update the minimum networkx), or should we have both pydot>=1.2.3 and pydotplus in the extras?

Also, we should set the minimum version on pydot to match networkx's.

@djarecka
Copy link
Collaborator Author

@effigies : I tried to write my PR in a way that it works with networkx < 2 and networkx > 2. What about I will keep both pydotplus and pydot?

@effigies
Copy link
Member

That's reasonable. It just assumes that we care enough about supporting multiple versions of networkx.

@effigies
Copy link
Member

Can you sync the networkx min versions, while you're at it? (rtd_)requirements says 1.7, info says 1.9. I'm assuming info is correct.

@djarecka
Copy link
Collaborator Author

changed it.

regarding multiple versions support, the 2.0 is brand new (I was using dev version for my PR), so @satra wanted to support them both. Not sure for how long we want to do it, but my point was that in this PR I did try to check my changes with both versions, so we can just keep pydotplus for some time.

@effigies
Copy link
Member

Sounds good. If this passes (and nipreps/niworkflows#196, nipreps/fmriprep#723), I'm 👍 for merging as is.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Downstream tests are passing.

Copy link
Contributor

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I have tested this with both networkx 1.x and 2.x and worked. Great job @djarecka !

@oesteban oesteban mentioned this pull request Sep 22, 2017
@oesteban
Copy link
Contributor

oesteban commented Sep 22, 2017

On your branch:

$ git grep "package_check('networkx"
examples/dmri_dtk_dti.py:package_check('networkx', '1.0', 'tutorial1')
examples/dmri_dtk_odf.py:package_check('networkx', '1.0', 'tutorial1')
examples/dmri_fsl_dti.py:package_check('networkx', '1.0', 'tutorial1')
examples/fmri_slicer_coregistration.py:package_check('networkx', '1.0', 'tutorial1')
nipype/pipeline/engine/utils.py:package_check('networkx', '1.3')
nipype/pipeline/engine/workflows.py:package_check('networkx', '1.3')
nipype/utils/misc.py:    package_check('networkx', '1.0', 'tutorial1')

We probably don't need any of those.

@effigies effigies mentioned this pull request Sep 22, 2017
@effigies
Copy link
Member

I don't understand what's changed to cause afni.preprocess.Hist to repeatedly stall.

@satra
Copy link
Member

satra commented Sep 23, 2017

@effigies - i have restarted the build several times yesterday with the same result, and it's only happening on 3.6.

i'll build a neurodocker environment locally to test.

@satra satra mentioned this pull request Sep 23, 2017
@satra satra closed this in #2196 Sep 23, 2017
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.

5 participants