search

tomchor / Oceanostics.jl

Diagnostics for Oceananigans
https://tomchor.github.io/Oceanostics.jl/
MIT License
24 stars 8 forks source link

Fixes `TracerVarianceDissipationRate` on GPUs #112

Closed tomchor closed 1 year ago

tomchor commented 1 year ago

Closes https://github.com/tomchor/Oceanostics.jl/issues/111

This PR also adds tracer variance budget testing for different closures.

tomchor commented 1 year ago

@glwagner can i merge this?

glwagner commented 1 year ago

I think so! I am slightly concerned this is type piracy, ie you are extending Oceananigans methods (like calling diffusive_flux_x with a closure tuple) in a way that does not specifically dispatch on any Oceanostics types.

I think the main issue here is that Oceananigans developers may not be aware that you have defined these methods here, which could cause Oceanostics and Oceananigans to interact poorly in the future.

Maybe the tupled versions of diffusive_flux_x, etc actually belong in Oceananigans? The problem is that Oceananigans testing infrastructure is way overburdened so I'm conflicted.

tomchor commented 1 year ago

I think so! I am slightly concerned this is type piracy, ie you are extending Oceananigans methods (like calling diffusive_flux_x with a closure tuple) in a way that does not specifically dispatch on any Oceanostics types.

I think the main issue here is that Oceananigans developers may not be aware that you have defined these methods here, which could cause Oceanostics and Oceananigans to interact poorly in the future.

Maybe the tupled versions of diffusive_flux_x, etc actually belong in Oceananigans? The problem is that Oceananigans testing infrastructure is way overburdened so I'm conflicted.

Yeah I see your point. I originally considered submitting a PR to include the tupled versions on Oceananigans. Although I think testing the tupled version wouldn't add too much of a burden to the tests, no?

In any case, TracertVarDissipation is failing on main on GPUs so this change is needed somewhere. If you prefer (and this is my preference) we can merge this PR as is for now. Then I'll open a PR on Oceananigans to see how much of a burden it would be to test this over upstream. And subsequently remove the method extension here if we decide to approve it there.

@glwagner I'll defer to you.

tomchor commented 1 year ago

I'm merging this and we can change it later if we decide to include that in Oceananigans. I may open a PR there soon