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

NetworkX compatibility #1673

Closed
richardjgowers opened this Issue Sep 24, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@richardjgowers
Member

richardjgowers commented Sep 24, 2017

One of our requirements networkx has released v2.0, which is an API break from 1.0. We should check instances where we use networkx (only analysis.leaflet) and check that our usage is future proof (or pin to version 1.x only)

I think @orbeckst wrote leaflet? But it's probably easy for anyone to do

Migration guide:
https://networkx.github.io/documentation/stable/release/migration_guide_from_1.x_to_2.0.html

@richardjgowers richardjgowers added this to the 0.17.0 milestone Sep 24, 2017

@tylerjereddy

This comment has been minimized.

Show comment
Hide comment
@tylerjereddy

tylerjereddy Sep 25, 2017

Member

Note that Aric Hagberg (one of the original authors) works here if you need me to ask him anything.

Member

tylerjereddy commented Sep 25, 2017

Note that Aric Hagberg (one of the original authors) works here if you need me to ask him anything.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Sep 25, 2017

Member

a short search reveals that we only use it in the leaflet code and all tests pass. Locally and I think on our CI as well. Conda-forge already has networkx v2.0

Member

kain88-de commented Sep 25, 2017

a short search reveals that we only use it in the leaflet code and all tests pass. Locally and I think on our CI as well. Conda-forge already has networkx v2.0

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Sep 25, 2017

Member
Member

orbeckst commented Sep 25, 2017

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Nov 30, 2017

Member

Is this still an issue or can we close it? test_leaflet seems to be passing so I would think that this is still ok.

Member

orbeckst commented Nov 30, 2017

Is this still an issue or can we close it? test_leaflet seems to be passing so I would think that this is still ok.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Nov 30, 2017

Member

my concern was leaflet coverage is 73%, so there's a fair chunk of space where an outdated networkx call could live

Member

richardjgowers commented Nov 30, 2017

my concern was leaflet coverage is 73%, so there's a fair chunk of space where an outdated networkx call could live

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Nov 30, 2017

Member
Member

kain88-de commented Nov 30, 2017

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Dec 1, 2017

Member

According to https://coveralls.io/builds/14437937/source?filename=%2Fhome%2Ftravis%2Fminiconda%2Fenvs%2Ftest%2Flib%2Fpython3.6%2Fsite-packages%2FMDAnalysis%2Fanalysis%2Fleaflet.py all the lines with networkX functionality are tested and the tests pass so I assume that we are using the latest networkx correctly.

I'll close this issue with no further actions. (Feel free to reopen if necessary.)

Member

orbeckst commented Dec 1, 2017

According to https://coveralls.io/builds/14437937/source?filename=%2Fhome%2Ftravis%2Fminiconda%2Fenvs%2Ftest%2Flib%2Fpython3.6%2Fsite-packages%2FMDAnalysis%2Fanalysis%2Fleaflet.py all the lines with networkX functionality are tested and the tests pass so I assume that we are using the latest networkx correctly.

I'll close this issue with no further actions. (Feel free to reopen if necessary.)

@orbeckst orbeckst closed this Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment