Skip to content

improve from_networkx performance #402

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

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

szhorvat
Copy link
Member

@szhorvat szhorvat commented May 27, 2021

This attempts to address #401. It only fixes from_networkx, not from_graph_tool. I need some feedback before proceeding. I don't really know much Python, and this was merely an exercise for me. Take a very careful look before merging.

@szhorvat szhorvat requested review from iosonofabio and ntamas May 27, 2021 18:25
@szhorvat
Copy link
Member Author

Benchmark:

import random
random.seed(123)
g = nx.gnm_random_graph(1000,10000)

then in ipython,

%timeit ig.Graph.from_networkx(g)

The speedup is from ~2.0 seconds to ~12 milliseconds.

@szhorvat
Copy link
Member Author

I notice that the name Graph.from_networx seems inconsistent with other graph constructors, all of which are capitalized. Shouldn't this be corrected?

@szhorvat
Copy link
Member Author

Why is the test for this functionality in src/igraph/test while everything else is in tests? Is this test run when doing python -m unittest as in the instructions?

Originally I messed up and forgot to convert vertex names to IDs, yet tests did not seem to fail. Assuming I ran the tests correctly, the tests should be improved to catch this mistake.

@ntamas
Copy link
Member

ntamas commented May 27, 2021

I notice that the name Graph.from_networx seems inconsistent with other graph constructors, all of which are capitalized. Shouldn't this be corrected?

The capitalization is a leftover from the early days of the development of the Python interface where I thought this would be a good idea to distinguish class methods from "normal" methods. Nowadays I don't use this convention in new code and prefer normal underscore_case instead. When we re-design the python-igraph API in the future such that we won't have a monolithic Graph class any more, this inconsistency will probably not be relevant any more. Until then, we can alias from_networkx with From_Networkx if we want to. Opinions?

Why is the test for this functionality in src/igraph/test while everything else is in tests?

Because I probably forgot to move them when I switched from the tests-within-the-package layout to the tests-outside-the-package layout as advocated here :) It should be in tests by all means.

@ntamas ntamas merged commit f6eb8cf into igraph:master May 27, 2021
@ntamas
Copy link
Member

ntamas commented May 27, 2021

Thanks a lot!

@szhorvat
Copy link
Member Author

Until then, we can alias from_networkx with From_Networkx if we want to. Opinions?

No preference from me, just want to note that if this is done, it should be From_NetworkX and not From_Networkx.

@iosonofabio
Copy link
Member

Thank you. Let's not rename and break API and also go against Python conventions.

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.

3 participants