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

Minor bug fix so that Δzᶜᶜᶜ(... , ibg::PCBIBG) works for grids with flat dims #3530

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Apr 1, 2024

There was a

x, y, z = node(...)

but only z was needed. And node returns a different-size tuple depending on how many dimensions of grid are non-flat. Thus there was issues sometimes. Now that part of the code became

z = znode(...)

which is much more robust!

@navidcy navidcy added bug 🐞 Even a perfect program still has bugs immersed boundaries ⛰️ Less Ocean, more anigans labels Apr 1, 2024
@glwagner
Copy link
Member

glwagner commented Apr 1, 2024

Nice! I think the node was a legacy from when the bottom height could be a function of x, y. This is better now that we know it's pretty much 100% of the time better to precompute that.

Copy link
Collaborator

@francispoulin francispoulin left a comment

Choose a reason for hiding this comment

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

Looks good!

I think in line 122 you need a .- instead of -, for the code to work.

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 1, 2024

I don't think so.
Now both h and z are just floats so the . you suggest is redundant.

@glwagner
Copy link
Member

glwagner commented Apr 1, 2024

I don't think you want .- --- the metric functions are invoked inside kernels, where broadcasting doesn't work on the GPU.

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 1, 2024

@francispoulin do you still insist it needs broadcasting? Note that you need to approve the PR otherwise it can’t be merged.

@francispoulin francispoulin dismissed their stale review April 1, 2024 23:40

I am happy to merge as is and I hope this will dismiss my review. If not I will try again.

@francispoulin
Copy link
Collaborator

@francispoulin do you still insist it needs broadcasting? Note that you need to approve the PR otherwise it can’t be merged.

Sorry for holding things up and please merge when you like.

@navidcy navidcy merged commit ff3ab99 into main Apr 2, 2024
48 of 49 checks passed
@navidcy navidcy deleted the ncc/partial-cell-fix branch April 2, 2024 05:15
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 grids 🗺️ immersed boundaries ⛰️ Less Ocean, more anigans
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants