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

Length check added to simple_cycles #4798

Closed
wants to merge 2 commits into from

Conversation

vakker
Copy link

@vakker vakker commented May 13, 2021

Adding a max_len parameter to simple_cycles according to #4797
Johnson's algorithm needs to be checked to see if this interferes with it.

@dschult
Copy link
Member

dschult commented Jun 12, 2021

Are you going to check whether this interferes with Johnson's algorithm?

@vakker
Copy link
Author

vakker commented Jun 14, 2021

So it's been discussed here but we haven't really reached a conclusion on how to combine the two.

@dschult
Copy link
Member

dschult commented Jun 14, 2021

It looks like that discussion ended here with a request that you construct a separate function for the length check version so that it wouldn't affect the runtime of the case max_len is None. Perhaps as a first check that this is true, you could time some graphs with your version and having max_len=None and the existing networkx version. Is there a difference in runtime?

If there is a difference, you could in-line the function call (python can be slow to call functions)
if nbrs and max_check_len(path): becomes:

if nbrs and (max_len is None or len(path) < max_len):

@rossbar
Copy link
Contributor

rossbar commented Dec 12, 2021

Gentle ping @vakker have you had a chance to investigate the suggestion above? I suspect that the version without the function call overhead would indeed be more performant, though it'd be interesting to see what the practical effects are (if any).

@vakker
Copy link
Author

vakker commented Dec 12, 2021

@rossbar sorry for the delay, it didn't really have time to investigate this further. Probably during the holidays or early next year I can wrap this up.

@vakker
Copy link
Author

vakker commented Jan 12, 2022

@rossbar sorry, I don't have the bandwidth at the moment to further investigate this thoroughly, but if anyone comes along it should be straight forward to pick it up. Maybe @boothby has something to add here?

@dschult
Copy link
Member

dschult commented May 21, 2023

Fixed in #6461 :)

@dschult dschult closed this May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants