Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Jun 11, 2023

Conversation

mjclobo
Copy link
Contributor

@mjclobo mjclobo commented Jun 9, 2023

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:

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

@navidcy
Copy link
Member

navidcy commented Jun 9, 2023

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:

version = "0.15.2"

to 0.15.3.

@navidcy navidcy added the 🐞 bug Something isn't working label Jun 9, 2023
@navidcy
Copy link
Member

navidcy commented Jun 9, 2023

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

"""
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
Copy link
Contributor Author

mjclobo commented Jun 9, 2023

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:

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
Copy link
Contributor Author

mjclobo commented Jun 9, 2023

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

"""
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
Copy link
Member

navidcy commented Jun 9, 2023

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
Copy link
Member

navidcy commented Jun 9, 2023

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.

Updating version number for patch release
@mjclobo
Copy link
Contributor Author

mjclobo commented Jun 9, 2023

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
Copy link
Member

navidcy commented Jun 10, 2023

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
Copy link
Contributor Author

mjclobo commented Jun 10, 2023

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 navidcy self-requested a review June 10, 2023 08:22
Copy link
Member

@navidcy navidcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 navidcy merged commit 2620251 into FourierFlows:main Jun 11, 2023
@navidcy
Copy link
Member

navidcy commented Jun 11, 2023

@mjclobo welcome to GeophysicalFlows :) !

@mjclobo mjclobo deleted the sign_error_fix branch June 11, 2023 15:34
@mjclobo
Copy link
Contributor Author

mjclobo commented Jun 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants