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:

GeophysicalFlows.jl/src/multilayerqg.jl

Line 367 in c0c42ce

CUDA.@allowscalar @views Qy[:, :, j] = @. Qy[:, :, j] - Fp[j] * (U[:, :, j+1] - U[:, :, j]) + Fm[j-1] * (U[:, :, j-1] - U[:, :, j])

[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:

GeophysicalFlows.jl/Project.toml

Line 5 in ef43c78

version = "0.15.2"

to 0.15.3.

[navidcy]

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

GeophysicalFlows.jl/test/test_multilayerqg.jl

Lines 155 to 166 in c0c42ce

	"""
	test_mqg_nonlinearadvection(dt, stepper, dev; kwargs...)
	Tests the advection term by timestepping a test problem with timestep dt and
	timestepper identified by the string stepper. The test 2-layer problem is
	derived by picking a solution q1f, q2f (with streamfunctions ψ1f, ψ2f) for which
	the advection terms J(ψn, qn) are non-zero. Next, a forcing Ff is derived such
	that a solution to the problem forced by this Ff is then qf.
	(This solution may not be realized, at least at long times, if it is unstable.)
	"""
	function test_mqg_nonlinearadvection(dt, stepper, dev::Device=CPU();
	n=128, L=2π, nlayers=2, μ=0.0, ν=0.0, nν=1)

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:

GeophysicalFlows.jl/Project.toml

Line 5 in ef43c78

version = "0.15.2"

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!

GeophysicalFlows.jl/test/test_multilayerqg.jl

Lines 155 to 166 in c0c42ce

	"""
	test_mqg_nonlinearadvection(dt, stepper, dev; kwargs...)
	Tests the advection term by timestepping a test problem with timestep dt and
	timestepper identified by the string stepper. The test 2-layer problem is
	derived by picking a solution q1f, q2f (with streamfunctions ψ1f, ψ2f) for which
	the advection terms J(ψn, qn) are non-zero. Next, a forcing Ff is derived such
	that a solution to the problem forced by this Ff is then qf.
	(This solution may not be realized, at least at long times, if it is unstable.)
	"""
	function test_mqg_nonlinearadvection(dt, stepper, dev::Device=CPU();
	n=128, L=2π, nlayers=2, μ=0.0, ν=0.0, nν=1)

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 #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]

This looks good to me!
I hope that after merging #293 and merging main here the docs will be able to built on this PR and we can merge.

[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 :)