Fixing sign error for mean meridional PV gradient `Qy` in `MultiLayerQG` module

[mjclobo]

This pull request addresses issue #328 by changing the sign of the the Fm term when calculating the mean meridional PV gradient Qy for interior layers of a multi-layer model, found on line 367 of multilayerqg.jl:

https://github.com/FourierFlows/GeophysicalFlows.jl/blob/c0c42cebd076b9cf828af1b956a08241a70febcc/src/multilayerqg.jl#L367

[navidcy]

thanks @mjclobo this looks good!

The tests pass both before and after your change! This implies that the tests are oblivious to the sign of that term in the Qy. I'll try to think of an additional test that would capture such mistake an perhaps add it in the PR.

Also, since this is a bug fix we should make a new patch release. Can you bump a patch release version number? This means change the package version at:

https://github.com/FourierFlows/GeophysicalFlows.jl/blob/ef43c785187dd7b379f9000b69a3e8ab3cbc8644/Project.toml#L5

to 0.15.3.

[navidcy]

There is a test that would have captured this but it only tests 2 layer configurations!

https://github.com/FourierFlows/GeophysicalFlows.jl/blob/c0c42cebd076b9cf828af1b956a08241a70febcc/test/test_multilayerqg.jl#L155-L166

And since the typo was in the interior points, it didn't capture it! I'll write another 3-layer version of this test.

[mjclobo]

thanks @mjclobo this looks good!

The tests pass both before and after your change! This implies that the tests are oblivious to the sign of that term in the Qy. I'll try to think of an additional test that would capture such mistake an perhaps add it in the PR.

Also, since this is a bug fix we should make a new patch release. Can you bump a patch release version number? This means change the package version at:

https://github.com/FourierFlows/GeophysicalFlows.jl/blob/ef43c785187dd7b379f9000b69a3e8ab3cbc8644/Project.toml#L5

to 0.15.3.

Okay! Being I don't have write access to the main project, I should make changes to my fork and make a PR to merge the change to the Project.toml file, right?

[mjclobo]

There is a test that would have captured this but it only tests 2 layer configurations!

https://github.com/FourierFlows/GeophysicalFlows.jl/blob/c0c42cebd076b9cf828af1b956a08241a70febcc/test/test_multilayerqg.jl#L155-L166

And since the typo was in the interior points, it didn't capture it! I'll write another 3-layer version of this test.

Ah, I see. Nice! Yes, the two-layer model generally seems to be the star of the show...I'm hoping to run ~15 layer models for a research project of mine, so I'm glad to be the guinea pig :)

[navidcy]

Okay! Being I don't have write access to the main project, I should make changes to my fork and make a PR to merge the change to the Project.toml file, right?

Yes, change the Project.toml file on your fork on this branch: https://github.com/mjclobo/GeophysicalFlows.jl/blob/sign_error_fix/Project.toml

[navidcy]

I added a test and it does fail on main branch! I'll push to your branch and hopefully it passes there! I'll do that later -- I need to head out now.

[mjclobo]

I added a test and it does fail on main branch! I'll push to your branch and hopefully it passes there! I'll do that later -- I need to head out now.

Okay, no rush! I'll keep an eye out for it.

[navidcy]

Ah, I see. Nice! Yes, the two-layer model generally seems to be the star of the show...I'm hoping to run ~15 layer models for a research project of mine, so I'm glad to be the guinea pig :)

unrelated to this PR but note that if you plan to run MultiLayerQG on GPU for more than 2 layers we first need to deal with https://github.com/FourierFlows/GeophysicalFlows.jl/issues/112

Otherwise the PV inversion is very slow!! For the 2-layer case we hardcoded the inversion in #270. But that’s not doable for 15-layers. :)

[mjclobo]

Ah, I see. Nice! Yes, the two-layer model generally seems to be the star of the show...I'm hoping to run ~15 layer models for a research project of mine, so I'm glad to be the guinea pig :)

unrelated to this PR but note that if you plan to run MultiLayerQG on GPU for more than 2 layers we first need to deal with #112

Otherwise the PV inversion is very slow!! For the 2-layer case we hardcoded the inversion in #270. But that’s not doable for 15-layers. :)

Thanks for the heads up! I plan on trying to run the models on CPU, since that's what's most available to me, but if that's too slow maybe I can work on this. It seems pretty straightforward, though ``pretty straightforward'' are usually famous last words..

[navidcy]

@mjclobo welcome to GeophysicalFlows :) !

[mjclobo]

@mjclobo welcome to GeophysicalFlows :) !

Thanks so much for all of your time/patience/help, @navidcy ! I hope to contribute more (and more smoothly) in the future :)