INCF / csa

The Python implementation of the Connection-Set Algebra
GNU General Public License v3.0
13 stars 17 forks source link

Python 3 support, TravisCI and Coveralls integration #2

Closed espenhgn closed 6 years ago

espenhgn commented 7 years ago

Hi @mdjurfeldt, It would be great if you CSA implementation could be supported also with Python 3, as I now use CSA in LFPy (https://github.com/espenhgn/LFPy/tree/csa) in order to create connectivity matrices for networks set up using LFPy itself (which should support Python 2.7/3.4-3.6).

Just a quick summary of what I did with your CSA tool:

Issues:



The tests run fine with Python 3.4-3.6, but I did not investigate further the different output. May have missed something. 
mdjurfeldt commented 7 years ago

Very nice! Thank you!

I think we should find the reason for the 2.7 test_gaussnet failure before merging. I can help out with this within a few days but can't look at this immediately.

espenhgn commented 7 years ago

That would be awesome. If I have time, I'll try and investigate in parallel, I agree it should be fixed.

espenhgn commented 7 years ago

Hi again @mdjurfeldt , I "fixed" the Python 2 build issue and testing on both Python 2 and 3 will now run (https://travis-ci.org/espenhgn/csa). I have no idea though of what the code change in https://github.com/INCF/csa/pull/2/commits/83e02910980f0cfd1c80b53c9a74d5d1a1d0aaf3 do though for better or worse, and would check this carefully. I also noticed that tests running on Python 3 is brittle, I do get every once in a while the test error: image

espenhgn commented 7 years ago

Hi again @mdjurfeldt . I looked now at the code again, and decided to remove the monkey-patched ComplementaryIntervalSet.__len__ method (commit 8191a0a), as it is not clear to me why an Exception should be raised because of infinite length of the class instantiation. Remember the class is inheriting class IntervalSet which has an integer length.

Commit 4f52f19 fixes the brittle test test_partitionRandomN.

All tests w. TravisCI run ok: https://travis-ci.org/espenhgn/csa/

espenhgn commented 6 years ago

Hi. I've added now spaces (list( to list () and similar in most places that I found, and sorted out the usage of this function csa.connset.cmpPostOrder. For Python 3, the function cmp is removed altogether and cmp is no longer a keyword argument to sorted and list.sort. See https://docs.python.org/3/howto/sorting.html#the-old-way-using-the-cmp-parameter for details. The rewrite of cmpPostOrder was based off some officially suggested replacement of the operation provided by the old cmp function.

I added back the monkey-patched csa.ComplementaryIntervalSet.__len__ method behind comments, but having it in there will still break some tests on Python 2 but not Python 3.

As for code style, I will soon provide a separate PR on that, turns out this autopep8 tool will format the code accd. to common Python practice without changing the functionality of CSA. But here, it only occluded the changes needed for Python 3 support.

espenhgn commented 6 years ago

Excellent. Thank you @mdjurfeldt .