-
Notifications
You must be signed in to change notification settings - Fork 1
Add support for device vectors through a workaround #23
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
base: master
Are you sure you want to change the base?
Add support for device vectors through a workaround #23
Conversation
tjhei
left a comment
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.
Can you add a test or two please?
| assert_dof_handler(dof_handler); | ||
|
|
||
| const unsigned int min_level = dst.min_level(); | ||
| const unsigned int min_level = transfer.min_level(); |
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.
Why this change?
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.
Oops, this is a mistake, this must have changed in the commits since I made the PR last time. I copied and pasted the whole file from my previous PR over and it must have overwritten this change. Should have double checked, I restored this to its current version.
| LinearAlgebra::distributed::Vector<Number, dealii::MemorySpace::Host>; | ||
|
|
||
| MGTransferMatrixFree( | ||
| const MGLevelObject<MGTwoLevelTransfer<dim, VectorTypeHost>> &mg_transfers, |
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.
It is a bit surprising that we initialize with host vectors here, right? That would mean if we make this work on device later, user code needs to change?
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.
Yes this is a good point but rather annoying to work around. These inputs are used to initialize the underlying MGTransferMatrixFree class that is defined on the host which does the "workaround" part of this. To switch these to be device elements we would need some method by which we could convert them to equivalent objects on the host which I'm not sure how easy or feasible that is.
| dst_host; | ||
| MGLevelObject<VectorTypeHost> src_host(src.min_level(), src.max_level()); | ||
|
|
||
| copy_to_host(dst_host, dst); |
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.
same here
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.
This is not safe to remove directly because "dst.copy_locally_owned_data_from()" inside copy_from_mg called on the host transfer complains that dst is inappropriately sized. I believe this is because it is initialized but never partitioned correctly and the copy_to_host() call is accomplishing this.
Once I partitioned the vector this was safe to remove.
| VectorTypeHost dst_host; | ||
| VectorTypeHost src_host; | ||
|
|
||
| copy_to_host(dst_host, dst); |
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.
same here
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.
Same as above.
| VectorTypeHost dst_host; | ||
| VectorTypeHost src_host; | ||
|
|
||
| copy_to_host(dst_host, dst); |
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 it is necessary though
4f6e401 to
b5f1aaf
Compare
…to host to execute transfer before sending back to device.
b5f1aaf to
934ea03
Compare
| // ------------------------------------------------------------------------ | ||
|
|
||
|
|
||
| // Check MGTransferMatrixFree by comparison with MGTransferPrebuilt on a |
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.
Update this comment to include device
|
|
||
| // build host reference | ||
| MGTransferMatrixFree<dim, Number, dealii::MemorySpace::Host> | ||
| transfer_host(mg_constrained_dofs); |
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.
It looks like you are using local smoothing multi grid here. Not wrong, but portable MatrixFree is currently only doing global coarsening.
No description provided.