[FIX] Rename cugraph-ops symbols (refactoring) and update GHA workflows to call pytest via python -m pytest
#3688
Conversation
naimnv
changed the title
naimnv
changed the title
naimnv
changed the title
naimnv
changed the title
tingyu66
changed the title
cugraph-ops symbols (refactoring) and update GHA workflows to call pytest via python -m pytest
| using base_vertex_t = std::decay_t<vertex_t>; | ||
| using base_edge_t = std::decay_t<edge_t>; |
There was a problem hiding this comment.
If we require decay here, should we better apply decay at the caller site? vertex_t implies it is a non-const non-volatile value type in our code base, and I think we should better maintain that convention.
There was a problem hiding this comment.
that makes sense, I don't know about cugraph conventions, so decay would not hurt in case someone does end up calling this with a reference type or const or volatile. It is mainly used for the type checks anyway.
Do you want me to remove this?
There was a problem hiding this comment.
I see, yeah, then, I don't care whether we apply decay here or not (I initially thought the caller is also our code, but if the caller is a user, then, I am not sure removing decay here is good or bad).
| "cugraph-ops sampling not yet implemented for different node and edge types"); | ||
|
|
||
| const auto ops_graph = detail::get_graph(graph_view); | ||
| return ops::graph::uniform_sample_csc(rng_state, |
There was a problem hiding this comment.
It was csr in the old code, now it is csr, is this intentional?
There was a problem hiding this comment.
yes, see below, this is part of the refactoring
| using base_vertex_t = std::decay_t<vertex_t>; | ||
| using base_edge_t = std::decay_t<edge_t>; |
There was a problem hiding this comment.
Same here, if decay is necessary, I think we should apply this in the caller site.
| ops::graph::fg_csr<EdgeTypeT> graph; | ||
| graph.n_nodes = gview.number_of_vertices(); | ||
| graph.n_indices = gview.number_of_edges(); | ||
| ops::graph::csc<EdgeTypeT, NodeTypeT> graph; |
There was a problem hiding this comment.
Just to double check. csr to csc here is intentional, right?
There was a problem hiding this comment.
yes indeed, part of the refactoring was to align more with general naming conventions
| // FIXME this is sufficient for now, but if there is a fast (cached) way | ||
| // of getting max degree, use that instead |
There was a problem hiding this comment.
Just for curiosity, is there any performance benefit in providing a tighter bound for max_in_degree?
There was a problem hiding this comment.
For neighborhood sampling, no. For other operations in cugraph-ops, yes.
|
/merge |
This PR:
cugraph-opssymbols and updates tests incugraph-dgland-pygbased on cugraph-ops refactoringpython -m pytest. This is to fix thepytest not found errorin log.