Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
support heterogenous fanout type #4608
base: branch-24.12
Are you sure you want to change the base?
support heterogenous fanout type #4608
Changes from 2 commits
0adb2fd
bb5a3e2
10fa86d
f904350
1fc32c3
8fc21f8
01a57f3
3a6aeb2
d2f6467
5d25155
80f8b86
50e0fc5
9f455bf
d114534
cf4a3ae
bc87b50
3013684
d6b6234
068b0a3
6920f65
760c5cd
1e0ef27
de79620
8c17009
7b95c5e
19fc765
e30766c
5dd66f2
4b2764c
4c1c610
fe35c80
a658b29
e52a38a
c416439
98d6c57
d7165af
579fd0a
5cdf40a
a8fbd9d
9d5b3dd
0358c6e
799c35d
ab8aa72
7d8b5ad
f2190ba
1e96dcf
36c25ad
4857b36
263b6ac
9dff3ab
33c8b3d
e357f42
6081978
73b3ffe
ea972f3
8822192
e02a513
d6cb1d5
54fa155
499e041
b571deb
f6c4ce3
e71660d
4e2c8cf
2458149
df3e4ff
d4847e4
dc2c9ba
c01f4e4
dabd0c8
95ca286
383bfc4
68fa2f1
18899ed
57e6f96
d34d85c
2aa0903
fa0cb88
990d2ed
d44a46f
a1a6180
36ce4fc
c0a618f
965001b
a005a19
11f9c40
6b547fb
14e9a99
aebfd08
b159e1b
3b0c016
da82567
5b1bbb4
6d69f88
79d4527
2a66928
8c3d871
ae92c9f
d7d6109
6b3ffbd
67d7d0a
3386230
413a577
22db98d
6a95852
4f5dc3e
334ce6d
9b4683a
8cb0c94
77303d5
117a4ec
ac66f13
114bf56
63a59ca
84face3
ab853e5
802d9b0
c5bec5f
4bd09f6
5d6cb34
24e31cb
7ca5d59
64b7edc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another option to explore.
Create a new function called neighbor_sample. Create it off of the biased sampling API, but with the following changes:
rng_state
parameter to be right after the handle (before the graph_view). This feels like a better standard place for the parameter.We can then mark the existing uniform_neighbor_sample and biased_neighbor_sample as deprecated. When we implement, the internal C++ implementation can just call the new neighbor_sample with the parameters properly configured. This makes it a non-breaking change (eventually we'll drop the old functions), but still keeps the code reuse increased.
Thoughts @seunghwak ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> In this case, we may update the existing non-heterogeneous fanout type sampling functions as well. i.e. combine the uniform & biased sampling functions. Not sure about the optimal balancing point between creating too many functions vs creating a function with too many input parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I guess we should avoid creating a too busy function (one function handling all different types of sampling based on the input arguments excessively using std::variant & std::optional) but we should also avoid creating too many functions... Not sure what's the optimal balancing point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, adding new parameters exponentially increase code complexity (too handle all possible combinations of optional parameters), we should better create separate functions. If supporting an additional optional parameter requires only a minor change in the API and implementation, we may create one generic function (or we may create one complex function that handles all different options in the detail namespace and multiple public functions calling this if this helps in reducing code replication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we take the same approach here. Create a new C API function called neighbor_sample, following the biased function definition. Add this parameter. Deprecate the other functions. In the implementation we can just check for nullptr (NULL).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just tag this with FIXME. I understand it's a note to yourself, but we probably should look at this for vertex_pairs as well... but it's beyond the scope of this PR.