-
Notifications
You must be signed in to change notification settings - Fork 9
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
Define cpu<->gpu conversions #2114
Conversation
a7e72fb
to
b5816bc
Compare
08ff758
to
4142535
Compare
I spoke with @juliasloan25 earlier today, and she and I both ran into a few issues, that I'll summarize here: Partially implemented mirrored gpu objectsWe have mirror objects (e.g., I'm generally in favor of preserving small constructor meta because of these nice properties, and I think it could offer us some alternative solutions. But I'm not sure it's worth the effort for this particular task, at this moment. I think the value of having convenient cpu<->gpu conversions is worth exploring the route of synchronizing the structs so that we can convert everything back and forth. ClimaComms device ambiguityIf we start with a |
f4513b9
to
4ec39fa
Compare
I also spoke with @Sbozzolo recently about fixing this, and I think the conclusion is that we'll keep the This way we can still keep some distance from the meta data issues, while still being able to switch between cpu and gpu. The way this works is:
Closes #2091. |
4ec39fa
to
5a42ce5
Compare
Closes #1296. |
4ec39fa
to
dcbec76
Compare
I think it would still be useful to provide a Maybe something like this? """
todevice(device, field)
Move `Field` field to the given `device`.
This is particularly useful to move `Field`s from CPUs to GPUs and viceversa.
If the `field` is already defined on the target device, return a copy.
"""
function todevice(device::ClimaComms.AbstractDevice, field)
return Adapt.adapt(ClimaComms.array_type(device), field)
end
todevice(::ClimaComms.CPUMultiThreaded, _) = error("Not supported") (I don't know if adapting returns a copy) This is so that downstream packages don't have to explicitely depend on Adapt and use it. I think Adapt comes with a learning curve and is not the most intuitive of the packages. The documentation does not make it clear how to use it and is not very beginner friendly. In addition to this, In any case, all of this (both the user facing aspects and the implementation details) should be documented in the documentation. |
Sure, I think we can do this since it's a separate name (we shouldn't have any ambiguities). Speaking of which, I need to fix the ambiguity in the tests.
I don't think that we can/should remove adapt as an extension because it's generic and lightweight. But, I agree that introducing |
One downside of defining a new method, |
It feels like |
Ah, Aqua is (rightfully so) complaining about piracy. Let's move those definitions to ClimaComms. |
I need to strip out the ClimaComms pieces. This PR will depend on CliMA/ClimaComms.jl#103. |
2ad0471
to
e960790
Compare
e960790
to
7ceda1a
Compare
Try to fix downgrade ci
7ceda1a
to
6ff39ad
Compare
The main functionality is fully implemented without the wrapper, so let's follow up with a wrapper in a subsequent PR. |
Can you make sure to add documentation in the next PR? You added a new feature and you clarified the role of certain structs, but this GitHub discussion is the only place where one can find information about them |
Yes, there are a handful of doc pieces I’d like to update, and I thought that it’d be easier to group them together |
I ran into this again today, and I think it's about time we fix support for this.