Skip to content

Conversation

akshitasure12
Copy link
Contributor

Fixes #137

Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parallel implementation can only run with nodes=None then I think can_run would be more appropriate here-- instead of should_run. Also, please always remember to document such details. Thanks!

@dschult
Copy link
Member

dschult commented Aug 22, 2025

The current code just implements what NetworkX implements when nodes is not None.
So I interpret this comment as saying we should put the check for nodes is None into the can_run function, and also remove from the parallel code any checking or handling of the nodes parameter. We know its value is None.

Is this what you are meaning @Schefflera-Arboricola ?

@Schefflera-Arboricola
Copy link
Member

The current code just implements what NetworkX implements when nodes is not None.

ok, then I think should_run makes sense here-- the existing should_run message isn't very clear about why this backend should not be run when nodes != None

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I made a suggestion for the error message.
But I wonder if this will be common enough to justify a function in policies. Perhaps it is better as a one-off should-run function. But then upon thinking more I could see many cases where simple splitting on the nodes makes sense when nodes is None and not if e.g. nodes is a single node (or even a small collection of nodes).

So, I think this is the right place for it.
I'll go ahead and approve it now, with final wording of the warning soon to be fixed.

akshitasure12 and others added 2 commits August 30, 2025 14:35
Co-authored-by: Dan Schult <dschult@colgate.edu>
@akshitasure12
Copy link
Contributor Author

akshitasure12 commented Aug 30, 2025

Thank you @Schefflera-Arboricola and @dschult!

I also removed the handling for the case when nodes is not None, since this scenario should never occur after adding should_run_if_nodes_none. However, this leads to test failures. It doesn’t seem ideal to keep code for a case that never runs in nx-parallel-- can this still be merged despite the failing tests?

@dschult
Copy link
Member

dschult commented Aug 30, 2025

The test failure shows the difference between "should_run" and "can_run". :)
We should leave the code in place, because while "should_run" says we should not run it, we still can (by specifying the backend parameters for example). If we want to remove that code we should make a can_run function that returns False in this case. Personally, I think we can leave the code in place and provide the should_run. But I can imagine others feeling differently. And we might change our minds on the approach in the future (still developing these kinds of traditions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

set should_run=False unless nodes is None
3 participants