-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix computation of barotropic streamfunction #1063
Conversation
This merge includes several fixes: * Most important, the computation of the BSF only includes one constraint on the mean BSF at boundary vertices above 45 deg. S. Previously, the computation attempted to make the BSF zero on all boundary vertices, which is not correct since the value on isolated boundaries (e.g. the coast of South America vs. Antarctica) should differ by the transport between them. * The full velocity (including bolus and submesoscale components) is used instead of just the resolved. * The mask for transport has been fixed to correctly identify the deepest level on an edge. The colormap has been changed (to blue-orange-div) to indicate a clear break with the incorrect BSF plots before this fix and to slightly improve the contrast at high absolute values of the BSF. The code has been linted to follow PEP8 conventions
TestingI have run this on the Here is the corresponding previous MPAS-Analysis run: |
@irenavankova, please feel free to have a look as well, since this fix affects your work, too. |
Test SuiteI ran the test suite and results are here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me @xylar!
Approved based on test results and code inspection.
Thank you for having a look, @milenaveneziani! |
thanks @xylar for diving into this so quickly. looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved based on test results and code inspection.
Thanks @maltrud. The sooner this gets fixed, the less embarrassing it will continue to be for me. |
This merge includes several fixes:
The colormap has been changed (to blue-orange-div) to indicate a clear break with the incorrect BSF plots before this fix and to slightly improve the contrast at high absolute values of the BSF.
The code has been linted to follow PEP8 conventions
Checklist
Testing
comment in the PR documents testing used to verify the changesFixes #1061