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

Remove Immersed map from Adapting the ImmersedBoundaryGrid #3690

Merged
merged 38 commits into from
Sep 17, 2024

Conversation

simone-silvestri
Copy link
Collaborator

This PR removes the adapting of the immersed map from the immersed boundary grid by shuffling some definitions around and making sure that the active cells map is always passed as an argument instead of being embedded in the grid.

This can possibly help with parameter space issues in complex kernels (see ClimaOcean, Issue#116).

This PR is still a draft because I would like to take the opportunity to add some docstring for the immersed map

@inline active_interior_map(grid::NamedTupleActiveCellsIBG, ::Val{:east}) = grid.interior_active_cells.east
@inline active_interior_map(grid::NamedTupleActiveCellsIBG, ::Val{:south}) = grid.interior_active_cells.south
@inline active_interior_map(grid::NamedTupleActiveCellsIBG, ::Val{:north}) = grid.interior_active_cells.north
@inline active_interior_map(grid::ActiveZColumnsIBG, ::Val{:surface}) = grid.active_z_columns
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the purpose of this system of functions or what a "named tuple active cells ibg" means, for example.

Also the function is called "map", but the object is called "cells". Which is it -- a map, or cells?

@glwagner
Copy link
Member

glwagner commented Aug 7, 2024

I think this is a good change. It might be nice to change some of the names of the functions as well, because I'm having a hard time understand the logic of them

@simone-silvestri simone-silvestri marked this pull request as ready for review August 7, 2024 22:48
@simone-silvestri simone-silvestri added the 🚨 DO NOT MERGE 🚨 IN BIG BOLD RED CAPS WITH FLASHING SIRENS label Sep 4, 2024
@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Sep 4, 2024

This PR should be ready, but I would like to add a test before merging it. I am planning to add it after next week, with more in-depth testing for the ImmersedBoundaryGrid

@simone-silvestri simone-silvestri removed the 🚨 DO NOT MERGE 🚨 IN BIG BOLD RED CAPS WITH FLASHING SIRENS label Sep 17, 2024
@simone-silvestri
Copy link
Collaborator Author

I have added some tests, and everything is passing, including some tests for distributed immersed boundary grids that compare active_cells_map = true solutions with active_cells_map = false solutions in the serial version so I am quite confident the implementation works.

@simone-silvestri simone-silvestri merged commit 10e9fb0 into main Sep 17, 2024
46 checks passed
@simone-silvestri simone-silvestri deleted the ss/remove-immersed-map-from-adapt branch September 17, 2024 21:39
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.

2 participants