Further optimize from_pandas_edgelist with cudf

[eriknw]

This continues #4525 (and this comment) to avoid copies and to be more optimal whether using pandas, cudf, or cudf.pandas. Notably, using s.to_numpy with cudf will return a numpy array, but cudf.pandas may return a cupy array (proxy).

Also, s.to_numpy(copy=False) (from comment) is not used, b/c cudf's to_numpy raises if copy=False. We get the behavior we want by not specifying copy=.

I don't know if this is the best way to determine whether a copy occurred or not, but this seems like a useful pattern to establish, because we want to make ingest more efficient.

CC @rlratzel

[rlratzel]

Maybe I'm missing something obvious, but couldn't the same behavior be achieved by just doing the following?

Suggested change

	if is_src_copied:
	src_indices = src_array
	else:
	src_indices = cp.array(src_array)
	src_indices = cp.array(src_array, copy=False)

[eriknw]

I think you meant:

        src_indices = cp.array(src_array, copy=not is_src_copied)

which should work as expected.

I found if-else branches more clear to use here, and easy for us to do given that we already have the booleans around.

[eriknw]

And once again, if you didn't catch it from one of my previous comments in previous PRs, numpy 2 changed semantics:
https://numpy.org/doc/stable/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword
so I think it's okay to be extra clear here.

[rlratzel]

Thanks for the explanation, but no, actually that snippet wasn't what I was thinking. But after re-reading again I think there's some necessary side effects that I was missing, but I'd rather not assume I know what they are. Let's chat offline and I'll update the comment afterwards.

[rlratzel]

I think my questioning of why not just use copy=False and therefore why _cp_iscopied_asarray is needed at all is because I didn't initially realize this function was intentionally making copies of the incoming dataframe series for the graph to own. That makes sense and I confirmed this with Erik offline (although we should probably think about a future improvement to prevent yet another copy happening when the PLC graph is made, but that can be for later) so my only request is a comment mentioning that. Perhaps something like this here:

# Now that the arrays have been extracted from the input
# dataframe, create copies the graph instance can own. If a
# copy already occurred during the extraction step above,
# don't copy again.

but maybe something even shorter and better, maybe just in the docstring for the function could simply be

This function will create a copy of the input dataframe data
source and target series to be owned by the returned Graph object.

[eriknw]

Thanks, code comments added. NetworkX doesn't share ownership with input objects, so I figure neither should we.

[rlratzel]

Thanks @eriknw , I really like the attention to detail with the tests! I just had one question (see review) and I'm wondering if the answer to it could significantly change the PR.

[rlratzel]

Thanks for meeting offline and clarifying things! I updated the review with the explanation. LGTM overall, but I have just one request for a comment to add.

[rlratzel]

I think my questioning of why not just use copy=False and therefore why _cp_iscopied_asarray is needed at all is because I didn't initially realize this function was intentionally making copies of the incoming dataframe series for the graph to own. That makes sense and I confirmed this with Erik offline (although we should probably think about a future improvement to prevent yet another copy happening when the PLC graph is made, but that can be for later) so my only request is a comment mentioning that. Perhaps something like this here:

# Now that the arrays have been extracted from the input
# dataframe, create copies the graph instance can own. If a
# copy already occurred during the extraction step above,
# don't copy again.

but maybe something even shorter and better, maybe just in the docstring for the function could simply be

This function will create a copy of the input dataframe data
source and target series to be owned by the returned Graph object.

[rlratzel]

Thanks for the new comments.

[rlratzel]

/merge

[rlratzel]

/merge