search

tomchor / Oceanostics.jl

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

Fix mean velocities not being used in KE dissipation calculation #169

Closed zhihua-zheng closed 5 months ago

zhihua-zheng commented 5 months ago

Added a function to remove user provided mean velocities from model.velocities in KineticEnergyDissipationRate.

tomchor commented 5 months ago

Thank you so much @zhihua-zheng! I made a couple of small comments and it should be ready to merge after that.

zhihua-zheng commented 5 months ago

Could you please add a quick test making sure that KineticEnergyDissipationRate works well when U, V, or W are passed? Let me know if you're confused about how to do that and I'll help you.

I actually don't how to do that. Some instructions would be great!

tomchor commented 5 months ago

Could you please add a quick test making sure that KineticEnergyDissipationRate works well when U, V, or W are passed? Let me know if you're confused about how to do that and I'll help you.

I actually don't how to do that. Some instructions would be great!

No problem. Can you let me know once you do the rest? Then I'll do the testing myself :)

tomchor commented 5 months ago

@zhihua-zheng I just realized I can't do the changes for you right now because you forked this repo and made the changes in your own forked repo, to which I don't have access. Can you give me access to your forked repo? That might make it easier for me to modify this PR and add tests.

I just gave you access to Oceanostics, so that in the future you can just create a new branch here every time you open a PR, which should be easier.

tomchor commented 5 months ago

@zhihua-zheng included a couple of tests that should be enough. Basically it tests that KineticEnergyDissipationRate is calculating the correct value without providing an average U, and then that it calculates the correct value with U provided. You can see that adding a test simply means adding the necessary statements to the file test/runtests.jl.

I think at this point this test will fail because viscosity(model.closure, model.diffusivity_fields) can return either a scalar or a Field, and right now the equation I included there will only work for fields. So that probably needs to be changed. I'm going on time off for the next week, so I just wanted to post this here as it is since I'm you can probably modify that and figure out how to make tests pass from here :)

zhihua-zheng commented 5 months ago

Test passed.

zhihua-zheng commented 5 months ago

Tests also passed on GPU. Also made some changes to let they pass for stretched grid and HydrostaticFreeSurfaceModel.

tomchor commented 5 months ago

@zhihua-zheng I just started registering a new Oceananigans version with the changes in https://github.com/CliMA/Oceananigans.jl/pull/3567 (0.90.14).

After it's online (I guess it takes a couple of hours) please update the Manifest.toml to the latest version. Also this only works with the latest Oceananigans now, right? If so please update the Project.toml's compat entry for Oceananigans to 0.90.14 and up. After that (and after tests pass) we should be good to merge :)

tomchor commented 5 months ago

If this has been tested in GPUs, feel free to merge!

tomchor commented 5 months ago

@zhihua-zheng you had mentioned on Slack that you were getting weird results with KineticEnergyDissipationRate when compared to IsotropicKineticEnergyDissipationRate. Since you merged, I'm assuming you resolve that. Can you quickly summarize what was going on? (If it's relevant)

zhihua-zheng commented 5 months ago

I updated Oceananigans, and it worked as expected.

tomchor commented 5 months ago

Awesome. Thanks for the contribution :)