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

Replace networkx by proprietary data structure and algorithms #115

Merged
merged 18 commits into from Apr 19, 2019

Conversation

Projects
None yet
3 participants
@janezd
Copy link
Contributor

commented Apr 12, 2019

Issue

This add-on uses networkx for

  • its basic data structure (which is inefficient because it is based on dictionaries),
  • reading files (which is so buggy that it works, basically, for Pajek, and even for this we need to pre-process the file before passing it to networkx),
  • computing some statistics (which is way too slow, because it's implemented in pure Python).

Networkx needs to be replaced with something better and more efficient. A few months ago I looked around and found nothing useful.

Also fixes #24, fixes #104.

Description of changes

This PR implements a new (and quite simple) data structure, based on sparse matrices, and ports the widgets to it.

Includes
  • Code changes
  • Tests
  • Documentation
@pavlin-policar

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

This looks great - networkx is pretty terrible. However, we should make sure removing networkx doesn't break the Louvain clustering widget.

The widget outputs an Orange graph if the addon is available and relies on networkx to compute the clustering. As far as I am aware, that is the only widget in core orange that works like that.

@janezd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

I will remove networkx only in the addon. Core widgets can still use it. I believe that some widgets in bioinformatics or single cell use networkx, too.

As for outputting data to add-on widgets, I can provide a function to transform networkx graph to the new graph.

I guess I will pack the new graph into a separate package. This will allow core and other add-ons to include it as dependency instead of checking (like Louvain) whether the add-on is installed. The package will also be useful when we/I also implement some algorithms on graphs.

@janezd janezd changed the title Replace networkx by proprietary data structure and algorithms [WIP] Replace networkx by proprietary data structure and algorithms Apr 13, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch 5 times, most recently from 256db32 to 31198e4 Apr 13, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch from 8650ddd to e213f2d Apr 17, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch 4 times, most recently from 2c5bb73 to 6534dc5 Apr 17, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch from 294371e to 863a81f Apr 19, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch from 863a81f to 71ec112 Apr 19, 2019

@janezd janezd force-pushed the janezd:remove-networkx branch from 71ec112 to 8d95676 Apr 19, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Apr 19, 2019

Codecov Report

Merging #115 into master will decrease coverage by 1.41%.
The diff coverage is 58.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
- Coverage   63.56%   62.14%   -1.42%     
==========================================
  Files          25       29       +4     
  Lines        2835     2800      -35     
==========================================
- Hits         1802     1740      -62     
- Misses       1033     1060      +27
Impacted Files Coverage Δ
...contrib/network/widgets/tests/test_OWNxExplorer.py 100% <ø> (ø) ⬆️
setup.py 96.96% <ø> (ø) ⬆️
orangecontrib/network/network/twomode.py 100% <100%> (ø)
orangecontrib/network/network/__init__.py 100% <100%> (ø)
orangecontrib/network/__init__.py 80% <100%> (-5.72%) ⬇️
...gecontrib/network/widgets/tests/test_OWNxGroups.py 98.79% <100%> (-0.07%) ⬇️
orangecontrib/network/widgets/ownxsinglemode.py 100% <100%> (ø) ⬆️
...ntrib/network/widgets/tests/test_OWNxClustering.py 100% <100%> (ø) ⬆️
orangecontrib/network/tests/test_twomode.py 94.44% <100%> (-3.71%) ⬇️
...ontrib/network/widgets/tests/test_OWNxGenerator.py 100% <100%> (ø) ⬆️
... and 24 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 21f409b...8d95676. Read the comment docs.

@janezd janezd changed the title [WIP] Replace networkx by proprietary data structure and algorithms Replace networkx by proprietary data structure and algorithms Apr 19, 2019

@janezd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

I'm merging this. I'd like to have the bulk porting done, and now separately improve individual parts.

  • It passes existing tests. It is probably still buggy, so we won't release a new version of add-on before new tests are added and widgets are tested manually.
  • Some functionality is missing - compared with the previous version. It doesn't generate a myriad of different graphs, and, more importantly, network analysis widget is rather poor. This will be fixed in subsequent pull requests.
  • Some functionality that couldn't be supported earlier (directed graphs, multimodal graph) is not supported yet. This will be improved on in future PRs, too.

@janezd janezd merged commit 685da3c into biolab:master Apr 19, 2019

1 of 3 checks passed

codecov/patch 58.49% of diff hit (target 63.56%)
Details
codecov/project 62.14% (-1.42%) compared to 21f409b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.