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

Refactor of spacings and correction of spacings-connected bugs #3955

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

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Nov 23, 2024

Correct spacings on an ImmersedBoundaryGrid
closes #3954
closes #3701

Should correct also partial cell bottom

Unification of general grid metrics
closes #3957

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Nov 25, 2024

I was thinking of using this PR to tackle also issue #3957. What do people think about it?
@ali-ramadhan @navidcy @glwagner @tomchor

@simone-silvestri simone-silvestri changed the title Correct spacings on an ImmersedBoundaryGrid Correct spacings on an ImmersedBoundaryGrid + Unification of general grid metrics Nov 25, 2024
@simone-silvestri simone-silvestri added the cleanup 🧹 Paying off technical debt label Nov 25, 2024
@simone-silvestri simone-silvestri changed the title Correct spacings on an ImmersedBoundaryGrid + Unification of general grid metrics Refactor of spacings and correction of spacings-connected bugs Nov 25, 2024
@simone-silvestri
Copy link
Collaborator Author

The PartialCellBottom example crashes with this bugfix. I suggest I just revert this PR not to close #3958 and we can tackle that in a new PR if that's ok.

@simone-silvestri
Copy link
Collaborator Author

In this PR there is a suggestion to improve the internals following #3957.
The exported functions are only the verbose ones, while the "mathy" ones are left for internal use.
Let me know what you guys think @ali-ramadhan @glwagner @tomchor

@tomchor
Copy link
Collaborator

tomchor commented Nov 27, 2024

The PartialCellBottom example crashes with this bugfix. I suggest I just revert this PR not to close #3958 and we can tackle that in a new PR if that's ok.

Sounds good to me!

In this PR there is a suggestion to improve the internals following #3957. The exported functions are only the verbose ones, while the "mathy" ones are left for internal use. Let me know what you guys think @ali-ramadhan @glwagner @tomchor

I'm also okay with this. Is this a breaking change?

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Nov 29, 2024

I'm also okay with this. Is this a breaking change?

I think it's not breaking. The metrics that were defined in AbstractOperations to be part of abstract operations are removed in this PR so what was

using Oceananigans.AbstractOperations: Ax
c = CenterField(grid)
cAx = c * Ax

can be done in this PR with

using Oceananigans.Operators: Ax
c = CenterField(grid)
cAx = c * Ax

However, since AbstractOperations imports the metric functions, the first version is still valid, despite the actual objects being different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
3 participants