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

Rotate vels #614

Closed
wants to merge 4 commits into from
Closed

Rotate vels #614

wants to merge 4 commits into from

Conversation

kshedstrom
Copy link

This adds an option to save true north ssu and ssv, required for our BOEM contract. There are similar changes to SIS2.

 - This is required for our BOEM-funded Arctic project. It needs
 something similar for SIS2 as well.
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

These changes look good to me, but I do have one change that I would like to request. In the upcoming set of revisions to give rotational symmetry with Fused-Multiply-Adds (FMAs), we are going to be writing expressions like Ax * Bx + Ay * By as (Ax * Bx) + (Ay * By) so that the compiler does not have the option of fusing the x- or y- parts of the expressions but not the other one. It would be helpful if you could revise the expressions for ssu_east and ssv_north to either add the parentheses to prevent FMAs from adding an asymmetry, or at least write these expressions so that the cos_rot terms come first in both cases. This is, of course, a diagnostic so perhaps we need not be so particular here, but for a new contribution like this, it is probably easier to make this small change now rather than coming back to it later.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

Thank you for this updated PR.

@marshallward
Copy link
Member

@kshedstrom Can you enable "Allow rebase merging" in the Settings of ESMG/MOM6? This would allow us to update this PR to the latest dev/gfdl.

@kshedstrom
Copy link
Author

It looks like it is already set. Is it not behaving that way?

@marshallward
Copy link
Member

I'm not able to update this PR. But let's try to sort this out privately.

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/23445 ✔️ 🟡

This requires an update to available_diags.000000

@marshallward
Copy link
Member

This PR was rebased into this commit: bb8a653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants