add node2vec C API implementation #2069
Conversation
ChuckHastings
added
3 - Ready for Review
improvement
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #2069 +/- ##
================================================
+ Coverage 73.51% 73.59% +0.08%
================================================
Files 156 156
Lines 10295 10316 +21
================================================
+ Hits 7568 7592 +24
+ Misses 2727 2724 -3
Continue to review full report at Codecov.
|
Pull out common cascading logic/error handling into a template function. This cleans up the existing C API implementations and will make new C API implementations a bit simpler. Note: #2069 uses the old paradigm. Depending on merge order we might want to modify the node2vec implementation to use this technique also. Can decide whether to do that in 2069 if this PR merges first, or if 2069 merges first we can either add that to this PR or leave it as tech debt to address later. Authors: - Chuck Hastings (https://github.com/ChuckHastings) Approvers: - Seunghwa Kang (https://github.com/seunghwak) URL: #2077
| : sampling_type_(sampling_type), p_(p), q_(q), use_alpha_cache_(use_alpha_cache) | ||
| { | ||
| } | ||
|
|
||
| sampling_params_t(int sampling_type, double p = 1.0, double q = 1.0, bool use_alpha_cache = false) |
There was a problem hiding this comment.
Why are we taking int sampling_type here but sampling_strategy_t sampling_type above? Better be consistent.
And more fundamentally, why do we have two functions seemingly doing the same thing?
There was a problem hiding this comment.
The original code (developed for C++) included a constructor that takes an int sampling_type and converts it into the enum. This original code is used by the current python/cython interface.
In implementing the C code, I wanted to use the enum directly (I think it's more clear in the calling code to use the enum). I added the new constructor to allow for construction from the enum directly. I intend to remove the old constructor once the python/cython code uses the new C API.
There was a problem hiding this comment.
Gotcha, and in that case, FIXME statement stating the intention will make this clearer.
cpp/src/c_api/random_walks.cpp
Outdated
| bool compress_result_; | ||
| double p_; | ||
| double q_; | ||
| cugraph_random_walk_result_t* result_{}; |
There was a problem hiding this comment.
Isn't it our convention to define member variables before functions? And don't forget {} at the end of variable names.
There was a problem hiding this comment.
Added the {}. Not sure what you mean by your question.
There was a problem hiding this comment.
What we typically have is
struct {
functions(); ...
variables; ...
};
but here
struct {
variables;...
functions(); ...
};
There was a problem hiding this comment.
Ah. I interpreted your question as the opposite - hence my confusion.
What I have generally seen in our code base is that when we define a class we define it as:
class {
public:
functions;
private:
private_functions;
variables;
};
But when we define a struct (essentially the same, only real difference is that everything is public) we define it as:
struct {
variables;
functions;
};
Admittedly, most of the structs that we define are to work around the nvcc problem with lambdas. My feeling (from a design perspective) was that this functor is really a heavier weight lambda. But you are correct in observing that it is more class-like. It does derive from an abstract struct, hence the need for a constructor as well as an operator()().
This is consistent across all of the C API functions. We can certainly change it if we like, but if we do we should fix the entire C API (and better to do it before we add too many more calls).
There was a problem hiding this comment.
And I guess the sampling_params_t is a struct but follows the class description in my above comment. So we have inconsistency there as well.
There was a problem hiding this comment.
Oh, yes, you're right, actually, I also do {vairables; operators();} for struct (i.e. functors). Seeing https://github.com/rapidsai/cugraph/pull/2069/files#diff-1264d5c0712616b080bb085897241ca6ad0bd8d4d132c83a75be4edf229ce988L28, I erroneously thought that we do functions first (do we consider constructors as something special, or should this better be class, or should we resolve inconsistencies in ordering?).
There was a problem hiding this comment.
@seunghwak - I'll make a note to consider this and write up some options for us to discuss.
|
@gpucibot merge |
|
@gpucibot merge |
Provides C API implementation wrapping node2vec.