search

tomchor / Oceanostics.jl

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

Excise anisotropic dissipation rates + generalize 3D isotropic dissipation rates #68

Closed glwagner closed 2 years ago

glwagner commented 2 years ago

This PR excises the anisotropic dissipation rates (since this is no longer supported by Oceananigans 0.71+), and generalizes the isotropic dissipation rates to work for a wider range of viscosities (importantly, viscosities that are specified as functions of space and time).

Supercedes #67.

Might have to fix some tests, just opening this to see what happens.

glwagner commented 2 years ago

Why do these functions accept location but then error if its not (Center, Center, Center). Wouldn't it be simpler not to permit this argument?

tomchor commented 2 years ago

Why do these functions accept location but then error if its not (Center, Center, Center). Wouldn't it be simpler not to permit this argument?

When originally discussing this with @ali-ramadhan he suggested that since the idea was to expand for other locations in the future. We do need to flag that these functions are only appropriate for CCC somewhere though

glwagner commented 2 years ago

I guess we can add a comment like 

# TODO: add support for other locations

and remove the kwarg, perhaps? When / if we support other locations, we'll put the kwarg back?

tomchor commented 2 years ago

I fixed the tests in this PR: https://github.com/tomchor/Oceanostics.jl/pull/67 but due to time (and since I was unsure of what the best course of action was at the time) I didn't actually modify the functions!

Maybe you should merge that branch?

I guess we can add a comment like

# TODO: add support for other locations

and remove the kwarg, perhaps? When / if we support other locations, we'll put the kwarg back?

Sure! I'm okay with that

glwagner commented 2 years ago

I fixed the tests in this PR: #67 but due to time (and since I was unsure of what the best course of action was at the time) I didn't actually modify the functions!

Hmmm I don't see the fixes. But note I've made widespread changes here so there will probably be conflicts...

glwagner commented 2 years ago

It looks like we don't have required approvals here? @tomchor feel free to merge when you feel satisfied.

tomchor commented 2 years ago

It looks like we don't have required approvals here? @tomchor feel free to merge when you feel satisfied.

Weird, you should have permission, and there's no minimal number of approvals. I know @ali-ramadhan has merged things in the past so I'm not sure that's not working for you.

I approved the changes. Can you check if you can merge now? If now I'll merge it myself

glwagner commented 2 years ago

I can merge, I just didn't want to without someone else looking.

tomchor commented 2 years ago

I can merge, I just didn't want to without someone else looking.

Ah, I see your point. Since mainly it's been just me developing this package I didn't impose the need for approvals when merging, since most of the time no one reviews the changes. But I appreciate it :)