Skip to content
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

adapt_structure for AbstractArray boundary conditions invokes parent #3286

Closed
glwagner opened this issue Sep 23, 2023 · 5 comments · Fixed by #3287
Closed

adapt_structure for AbstractArray boundary conditions invokes parent #3286

glwagner opened this issue Sep 23, 2023 · 5 comments · Fixed by #3287
Labels
bug 🐞 Even a perfect program still has bugs

Comments

@glwagner
Copy link
Member

This prevents Field and OffsetArray from being used as boundary conditions (correctly) --- if users try to use Field, they get different behavior on CPU vs GPU.

I'm not sure the motivation for this choice.

Adapt.adapt_structure(to, b::BoundaryCondition{C, A}) where {C<:AbstractBoundaryConditionClassification, A<:AbstractArray} =
BoundaryCondition(C, Adapt.adapt(to, parent(b.condition)))

@glwagner glwagner added the bug 🐞 Even a perfect program still has bugs label Sep 23, 2023
@glwagner glwagner changed the title adapt_structure for BoundaryCondition abstract arrays uses parent adapt_structure for AbstractArray boundary conditions invokes parent Sep 23, 2023
@glwagner
Copy link
Member Author

It looks like it dates back to ancient times practically before the invention of writing

7f4b78e#diff-ef43fe13b0bf0810d1bd93a5cd3264c3be46ba792948fad6a924a62b281e3286

I'm amazed nobody has caught this bug before.

@navidcy
Copy link
Collaborator

navidcy commented Sep 26, 2023

It may be the answer to several conundrums. Or simply not many use fields for BCs?

@glwagner
Copy link
Member Author

@simone-silvestri may have tried. Not sure. @yuchenma23?

@yuchenma23
Copy link

Hi Greg, I was trying field as boundary condition but with time_interpolated_array Simone defined, which is used to interpolate field as a function of time. Essentially it's still a field boundary condition. It never worked on GPU with the bug that is related with some mismatch between types things. But it did work on CPU.

I then got to work on other stuff and never went back to check the code. Perhaps the bug you fixed could explain the bug I faced.

@glwagner
Copy link
Member Author

glwagner commented Oct 2, 2023

The fact that time_interpolated_array never worked on GPU explains why you didn't run into this problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Even a perfect program still has bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants