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

Broadcasting and operations on different Field types #12

Closed
ali-ramadhan opened this issue Dec 8, 2018 · 5 comments
Closed

Broadcasting and operations on different Field types #12

ali-ramadhan opened this issue Dec 8, 2018 · 5 comments
Assignees
Labels
abstractions 🎨 Whatever that means

Comments

@ali-ramadhan
Copy link
Member

Right now I've decided to not implement broadcasting for Field types. It would be really nice but I'd have to figure out how to take into account the different field types, e.g. can broadcast T .+ S as they're both of type CellField but not u .+ v as u::FaceFieldX, v::FaceFieldY.

Maybe even take it further and implement u .+ v which would have to do u .+ avgx(avgy(v)), and isn't commutative anymore.

@glwagner
Copy link
Member

glwagner commented Dec 8, 2018

Maybe we should take a step back and think about what our goal is? What do we want from this system? Will this simplify the MITgcm algorithm?

@ali-ramadhan
Copy link
Member Author

I think broadcasting for same-field operators, e.g. CellField .+ CellField would be really useful and make the code more readable. The simplification here is fewer loops to look at which is a big plus I think.

But cross-field broadcasting is a bit trickier so might be better to leave it unimplemented especially as it's not a very common operation, I only use it when adding f .* v to Gu or f .* u to Gv. Without it you would have to explicitly write u .+ avgx(avgy(v)) so if you forget it then it might make finding the bug a bit harder. With it, you can just write u .+ v with the averaging happening behind the scenes, so you might apply it by mistake without realizing it. Another potentially difficult bug to find.

I feel like everyone has a different opinion on this but in my mind, I'm thinking this would make the code easier to read and write (short-term), and ultimately easier to extend by users (long-term).

I just sometimes use issues leave random TODO notes and ramble.

@ali-ramadhan ali-ramadhan added feature 🌟 Something new and shiny wontfix labels Feb 9, 2019
@ali-ramadhan
Copy link
Member Author

Will not implement cross-field operations for now, e.g. FaceFieldX .+ CellField. Too complex and rarely used.

@ali-ramadhan ali-ramadhan added abstractions 🎨 Whatever that means and removed feature 🌟 Something new and shiny labels Mar 2, 2019
@ali-ramadhan ali-ramadhan self-assigned this Mar 17, 2019
@ali-ramadhan ali-ramadhan changed the title Broadcasting and addition of Field types should take into account the different field types. Broadcasting and operations on different Field types May 31, 2019
@ali-ramadhan
Copy link
Member Author

Reopening as this is becoming relevant for online diagnostics which may need a lot of these operations. E.g. w*T as @glwagner points out.

@ali-ramadhan
Copy link
Member Author

This was resolved in PR #463.

ali-ramadhan pushed a commit that referenced this issue Jan 13, 2020
Bootstrap package use PkgTemplates.jl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means
Projects
None yet
Development

No branches or pull requests

2 participants