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

Cute compute! for AbstractOperation #2235

Closed
wants to merge 1 commit into from
Closed

Cute compute! for AbstractOperation #2235

wants to merge 1 commit into from

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Feb 8, 2022

Might be useful at the REPL for interactive stuff cause you can write

julia>= compute!* ∂z(T) - β * ∂z(S))

for example.

If people like, I'll add docs and a test or two.

I guess the equivalent one-liner right now is

julia>= @compute Field* ∂z(T) - β * ∂z(S))

and the equivalent two-liner is

julia>= Field* ∂z(T) - β * ∂z(S))
julia> compute!(N²)

@glwagner
Copy link
Member Author

glwagner commented Feb 8, 2022

Bah, I realized this won't work because we have to call compute! on all leaves of an expression tree. For that it's important that compute!(::AbstractOperation) doesn't do anything.

We can support this if we change that interface, eg if we add something compute_leaf!. Then we can adapt compute! for public use and update the private methods like compute_leaf! as needed. Might not be worth it, probably there's other stuff we also need to work on if we want totally beautiful REPLness.

@glwagner glwagner marked this pull request as draft February 8, 2022 19:08
@tomchor
Copy link
Collaborator

tomchor commented Feb 11, 2022

I guess the equivalent one-liner right now is

julia>= @compute Field* ∂z(T) - β * ∂z(S))

Honestly I think this is already pretty handy and enforces the idea that in order to compute something it needs to be a field. When I was starting with Oceananigans it took me a while to understand that I couldn't put abstract operations into the output writers because the computation would fail. I think something like you're proposing would have added to my confusion.

Also, I only recently became aware that this existed. Maybe we should document the @compute macro somewhere in the docs?

@glwagner
Copy link
Member Author

it took me a while to understand that I couldn't put abstract operations into the output writers

This makes sense to me --- after all, who cares what a Field is? We want to write a computation to disk. Maybe we should support AbstractOperations and Reduction as output directly, to save the boilerplate of always wrapping things in Field? It occurs to me that the majority of users don't really need to know what a Field is (at least not when they're starting out).

@glwagner
Copy link
Member Author

I think something like you're proposing would have added to my confusion.

Isn't the confusion a problem with the output writers API? I think it sounds like a great idea to support AbstractOperation output. It's kind of logical. Doing this even allows us to do some clever stuff behind the scenes, like using one underlying array to store the results of multiple computations performed serial (thus saving memory).

@glwagner
Copy link
Member Author

Honestly I think this is already pretty handy and enforces the idea that in order to compute something it needs to be a field.

Hmm, I think the key concept here is that in order to store the result of a computation, we need to allocate memory. That's what invoking Field does.

It's worth mentioning for posterity the subtlety that calling compute! does have an affect on abstract operation, because it triggers the computation of all the leaves. So if an AbstractOperation depends on a field that needs to be computed, calling compute! on the abstract operation will cause that child field to get computed.

@glwagner
Copy link
Member Author

I'll close this PR because, in addition to it being unpopular, there's too much work we need to do to support it. Maybe something will come out of #2242 though.

@glwagner glwagner closed this Feb 11, 2022
@tomchor
Copy link
Collaborator

tomchor commented Feb 11, 2022

I think something like you're proposing would have added to my confusion.

Isn't the confusion a problem with the output writers API? I think it sounds like a great idea to support AbstractOperation output. It's kind of logical. Doing this even allows us to do some clever stuff behind the scenes, like using one underlying array to store the results of multiple computations performed serial (thus saving memory).

The confusion is that I wasn't aware that that a Field has data, and therefore takes up memory, which allows it to store values. While an AbstractOperation is just instructions on how to compute things. This extended to the output writers, but I wouldn't say output writers were the source of confusion.

If we make it so that users don't have to know what a Field is and we can use abstract operations everywhere, then my comment is moot.

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.

None yet

2 participants