Skip to content

Use worksize for folded-grid extents#392

Open
taimoorsohail wants to merge 5 commits into
mainfrom
ts/size-to-worksize-to-avoid-halo-blowup
Open

Use worksize for folded-grid extents#392
taimoorsohail wants to merge 5 commits into
mainfrom
ts/size-to-worksize-to-avoid-halo-blowup

Conversation

@taimoorsohail

Copy link
Copy Markdown
Collaborator

Summary

  • replace four suspicious size(grid) extent calculations with worksize(grid)
  • align folded-grid setup paths with RightFaceFolded launch semantics
  • keep the change scoped to the pressure, ERA5, bathymetry, and tracer-diffusion paths we identified

Why

On RightFaceFolded grids, worksize(grid) includes the extra folded northern row while size(grid) does not. These setup paths could therefore build auxiliary fields over 1:Ny while runtime kernels later operate over the folded 1:Ny+1 working domain, which is a plausible source of seam-local instability.

Impact

This keeps extent calculations consistent with folded-grid kernel launches and should reduce the risk of mismatched north-fold state near the seam.

Validation

  • inspected the diff to confirm the change is limited to the four intended sites
  • did not run local tests, per repository instructions

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/Atmospheres/hydrostatic_pressure.jl 0.00% 26 Missing ⚠️
src/Bathymetry/regrid_bathymetry.jl 0.00% 1 Missing ⚠️
src/InitialConditions/diffuse_tracers.jl 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment thread src/Atmospheres/hydrostatic_pressure.jl Outdated
Comment on lines 70 to 80

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this seems like something for a kernel launch?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah probably this should be a kernel, I think claude really doesn't like kernels

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This loop is in main, not introduced by this PR, so I think this is an existing bug (not defending Claude of course ;) )

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Have a look now @simone-silvestri @navidcy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good

Comment thread src/DataWrangling/ERA5/ERA5_prescribed_atmosphere.jl Outdated
@taimoorsohail taimoorsohail marked this pull request as ready for review July 1, 2026 04:17
@navidcy

navidcy commented Jul 2, 2026

Copy link
Copy Markdown
Member

The CI breaks.

Was wondering, do we want to change the Base.size definition?
But then Base.size gives the number of cells, right? So perhaps we want to have both size and worksize?

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.

3 participants