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

Simplify kernel launching and add exclude_periphery option #3814

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Oct 2, 2024

@glwagner glwagner mentioned this pull request Oct 2, 2024
@glwagner glwagner requested a review from jagoosw October 3, 2024 21:25
@glwagner
Copy link
Member Author

glwagner commented Oct 3, 2024

@jagoosw can you review this?

@navidcy
Copy link
Collaborator

navidcy commented Oct 5, 2024

Distributed tests actually fail with errors :(

Copy link
Collaborator

@jagoosw jagoosw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yeah that all makes sense. I agree the renaming makes sense it was just hard to see in the file changes which was new code vs renamed things. LGTM!


if !isnothing(active_cells_map) # everything else is irrelevant
workgroup = min(length(active_cells_map), 256)
worksize = length(active_cells_map)
Copy link
Collaborator

@simone-silvestri simone-silvestri Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably need to keep this here for non-active domains

Suggested change
worksize = length(active_cells_map)
worksize = length(active_cells_map)
if worksize == 0
return nothing
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok nothing, I see it has moved in the launch!

@simone-silvestri
Copy link
Collaborator

I am not sure about the purpose of this PR. What is the reason we do not want to calculate the periphery in the nonhydrostatic model tendencies?
If there is a specific reason, does this solution also apply to immersed boundaries with active cell map? It seems like nothing changed for that case.

@simone-silvestri
Copy link
Collaborator

Also I am not sure about renaming to buffer. What is called buffer here is not really a buffer but a halo-dependent region of the active domain. What about calling it compute_halo_dependent_tendencies!?

@glwagner
Copy link
Member Author

glwagner commented Oct 7, 2024

I am not sure about the purpose of this PR. What is the reason we do not want to calculate the periphery in the nonhydrostatic model tendencies?

If there is a specific reason, does this solution also apply to immersed boundaries with active cell map? It seems like nothing changed for that case.

Because some open boundary conditions integrate an alternative PDE on the boundaries (eg for radiation conditions).

In other words the algorithm we are using is only valid for simple open boundary conditions (we designed it for impenetrable condition). More generally, the algorithm is wrong.

There are two other benefits. First, we do not use the tendencies on the periphery. So this increases the clarity of the code. Previously, it might not be obvious that even though we compute tendencies on the boundary, we overwrite the field values in fill_halo_regions!.

It saves a little bit of computation as well...

@glwagner
Copy link
Member Author

glwagner commented Oct 7, 2024

Also I am not sure about renaming to buffer. What is called buffer here is not really a buffer but a halo-dependent region of the active domain. What about calling it compute_halo_dependent_tendencies!?

This PR defines the "buffer" as the halo dependent region of the domain.

Before this PR there was no definition of this region at all, so this PR simply introduces one possible name. I am ok with a different name, but I think we should do it in another PR.

@simone-silvestri
Copy link
Collaborator

Also I am not sure about renaming to buffer. What is called buffer here is not really a buffer but a halo-dependent region of the active domain. What about calling it compute_halo_dependent_tendencies!?

This PR defines the "buffer" as the halo dependent region of the domain.

Before this PR there was no definition of this region at all, so this PR simply introduces one possible name. I am ok with a different name, but I think we should do it in another PR.

Ok, can we leave the refactoring to another PR than to clean up this one?

@glwagner
Copy link
Member Author

glwagner commented Oct 8, 2024

This should be done already (I'm not sure why the tests keep failing, its annoying).

I made the change because I seriously could not understand the code. I kept getting confused between "compute boundary tendencies" (which adds fluxes to the tendencies, coming in from the boundaries), and "compute tendencies_boundaries", which did something completely different (compute the halo dependent tendencies). So I started by changing the names so I could reason about the code without my head spinning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants