tulip-control / omega

Specify and synthesize systems using symbolic algorithms
https://pypi.org/project/omega
Other
46 stars 5 forks source link

Change node to nodes for network2.x compatibility #10

Closed tichakornw closed 4 years ago

tichakornw commented 4 years ago

A minor change to games/enumeration.py to change node to nodes. This should be compatible with both NetworkX1.x and NetworkX2.x according to

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

murrayrm commented 4 years ago

Do we need to update either requirements.txt or setup.py to put in the version number for networkx? It looks like current version requires >=2.0, but presumably we need something higher?

Also, Travis CI is failing in Python 3.5 and 3.6, but OK in 2.7 and 3.4. Not sure why that would happen. Sample error:

ERROR: fixpoint_test.test_ee_image_only_sys
675----------------------------------------------------------------------
676Traceback (most recent call last):
677  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
678    self.test(*self.arg)
679  File "/home/travis/build/tulip-control/omega/tests/fixpoint_test.py", line 53, in test_ee_image_only_sys
680    g.add_path([0, 1, 2])
681AttributeError: 'TransitionSystem' object has no attribute 'add_path'
slivingston commented 4 years ago

I am able to reproduce the error locally. I am investigating now.

murrayrm commented 4 years ago

From an e-mail from Nok: "For python2.7 and 3.4 that pass, they use networkx2.2. For python3.5, it uses networkx2.4. Some function is missing like add_path. There are also a number of "node" that needs to be changed to "nodes" for networkx2.4 to work."

slivingston commented 4 years ago

Yes, add_path is now a function rather than a method of a class. The corresponding upstream issue is https://github.com/networkx/networkx/issues/1883

@tichakorn832 I can finish making the changes here, including add_path, if you are busy.

slivingston commented 4 years ago

Documentation about the new add_path is available at https://networkx.github.io/documentation/stable/reference/generated/networkx.classes.function.add_path.html

slivingston commented 4 years ago

@tichakorn832 I do not know if you received a notification, but I was able to push a commit directly onto your fork.

Do we need to keep compatibility with NetworkX version 1 ?

I need to check feasibility; for example, Tulip is also affected.

murrayrm commented 4 years ago

Nok is working the networkx version issue in a branch on TuLiP. See https://github.com/tulip-control/tulip-control/pull/220.

tichakornw commented 4 years ago

Hey. Sorry for my late response. For whatever reason, I didn't get any notification from GitHub.

Thanks for the changes @slivingston. They all look good.

tichakornw commented 4 years ago

@murrayrm With @slivingston 's fixes, CI now passes. I think networkx >= 2.0 should work. However, I typically prefer to do == or both >= and <= where I know that everything within the bound actually works, rather than >= because of the failure like we had with python 3.5 VS python 3.4. We don't know when an external library is going to update their interface so having <= will help safeguard against it. The downside though is that we need to monitor this to make sure that we're not too behind their latest version.