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 user interface for KernelFunctionOperation #2964

Merged
merged 9 commits into from
Mar 15, 2023

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Mar 8, 2023

This PR gets rid of the kwargs computed_dependencies and parameters. Instead we just have "arguments", which are varargs to the constructor:

    using Oceananigans.Operators: ζ₃ᶠᶠᶜ # called with signature ζ₃ᶠᶠᶜ(i, j, k, grid, u, v)

    grid = model.grid
    u, v, w = model.velocities

    ζ_op = KernelFunctionOperation{Face, Face, Center}(ζ₃ᶠᶠᶜ, grid, u, v)

compute! on KernelFunctionOperation calls compute! on all of the kernel arguments.

I think this is a simpler and more intuitive interface leading to more understandable code.

@glwagner glwagner added the cleanup 🧹 Paying off technical debt label Mar 8, 2023
@navidcy
Copy link
Collaborator

navidcy commented Mar 8, 2023

Looks good to me. It's breaking change so bump up minor release..

@simone-silvestri
Copy link
Collaborator

sounds good

@navidcy navidcy changed the title Simplify user interface for KernelFunctionOperation Simplify user interface for KernelFunctionOperation Mar 8, 2023
@tomchor
Copy link
Collaborator

tomchor commented Mar 8, 2023

Looks good to me. It's breaking change so bump up minor release..

At the moment we have 3 PRs that are breaking releases: this one, #2963, and #2842.

If it looks like all of them are going to be merged, can we coordinate and tag/register v0.80.0 only after all these are merged?

@glwagner
Copy link
Member Author

glwagner commented Mar 8, 2023

Good idea

@glwagner
Copy link
Member Author

glwagner commented Mar 8, 2023

Is #2842 close?

@tomchor
Copy link
Collaborator

tomchor commented Mar 8, 2023

Is #2842 close?

I think so. I have a couple of @navidcy's comments left to address (which shouldn't be hard) and I'd appreciate another review from you before merging.

I could have it ready by tomorrow assuming no major points of disagreement or major changes emerge during the review process.

@navidcy
Copy link
Collaborator

navidcy commented Mar 9, 2023

some tests didn't make it through

@navidcy
Copy link
Collaborator

navidcy commented Mar 14, 2023

something seems to have broken for GPU?

@navidcy
Copy link
Collaborator

navidcy commented Mar 15, 2023

No need to bump release since we didn’t register v0.80.0 yet

@navidcy navidcy merged commit 65f756a into main Mar 15, 2023
@navidcy navidcy deleted the glw/kernel-function-args branch March 28, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants