[FIX] Initialize force_atlas2 old_forces device_uvector, use new rmm::exec_policy#1625
Conversation
cpp/src/layout/barnes_hut.hpp
Outdated
| // FA2 requires degree + 1 | ||
| rmm::device_uvector<int> d_massl(nnodes + 1, stream); | ||
| thrust::fill(rmm::exec_policy(stream)->on(stream), d_massl.begin(), d_massl.end(), 1.f); | ||
| thrust::fill(rmm::exec_policy(stream), d_massl.begin(), d_massl.end(), 1.f); |
There was a problem hiding this comment.
Quick question. Never noticed this in earlier reviews of this code.
Is it intentional that we use float for these values when the graph might have float or double input types? Should we be templating these float instances to be weight_t do avoid the loss of value in conversions, or is it really the case that a graph with double weights should be using float for these values being computed?
I don't know enough about the context of this algorithm to know the answer, please enlighten.
There was a problem hiding this comment.
Looks like edge weights are only used in apply_attraction() which discards the extra precision, so I don't think it matters.
We should be filling with 1 instead of 1.0f though, because d_massl is a device_uvector of integers.
There was a problem hiding this comment.
OK. We should probably have a bigger conversation about this, but I don't think it should hold up progress here.
Could you add the following circa line 94:
// FIXME: Review whether these should be "float" or "weight_t"
That way we can come back at some point (perhaps when we refactor with the new graph) and revisit this.
There was a problem hiding this comment.
Two main reasons why I'm against it:
- Double-precision arithmetic in the force integration kernels probably isn't worth the performance hit.
- A user could accidentally opt in to a much slower implementation by accidentally using FP64 weights.
Codecov Report
@@ Coverage Diff @@
## branch-21.06 #1625 +/- ##
===============================================
Coverage ? 59.75%
===============================================
Files ? 79
Lines ? 3484
Branches ? 0
===============================================
Hits ? 2082
Misses ? 1402
Partials ? 0 Continue to review full report at Codecov.
|
BradReesWork
added
3 - Ready for Review
improvement
|
rerun tests |
|
@gpucibot merge |
| cudaStream_t stream = handle.get_stream(); | ||
| const edge_t e = graph.number_of_edges; | ||
| const vertex_t n = graph.number_of_vertices; | ||
| rmm::cuda_stream_view stream(handle.get_stream()); |
There was a problem hiding this comment.
Seems like we should use get_stream_view() for this, no? https://github.com/rapidsai/raft/blob/d5763480b899aa572afa92432fdf5bd6626a98a0/cpp/include/raft/handle.hpp#L102
There are a couple of other occurrences in the PR
|
@gpucibot merge |
1 similar comment
|
@gpucibot merge |
6 participants
Fixes an issue where the layout for all points converge to
(0,0)when running multiple layout ticks after #1607.d_old_forcesvector to fix layout issue.barnes_hut.hppto use non-deprecatedrmm::exec_policy.