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

FMS_cap: extract c-grid staggered currents #1611

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

sanAkel
Copy link
Collaborator

@sanAkel sanAkel commented Oct 7, 2023

This PR adds an option:

  • In FMS_cap to extract uc and vc currents.
  • Was added by @zhaobin74; he is working on using these exports in GEOS-ESM CICE6.
  • By construction does not change answers.

Notes:

@jiandewang, @gustavo-marques, @alperaltuntas and @abozec,
AFAIK you do not use FMS_cap, so hopefully this is irrelevant to your use cases.

@marshallward, @Hallberg-NOAA, @kshedstrom:
Appreciate your comments.

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #1611 (df26ff7) into main (2dd99f8) will not change coverage.
Report is 5 commits behind head on main.
The diff coverage is n/a.

❗ Current head df26ff7 differs from pull request most recent head a2c3ec7. Consider uploading reports for the commit a2c3ec7 to get more accurate results

@@           Coverage Diff           @@
##             main    #1611   +/-   ##
=======================================
  Coverage   38.17%   38.17%           
=======================================
  Files         269      269           
  Lines       76503    76503           
  Branches    14064    14064           
=======================================
  Hits        29202    29202           
  Misses      42054    42054           
  Partials     5247     5247           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@kshedstrom kshedstrom left a comment

Choose a reason for hiding this comment

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

I approve this PR.

Copy link
Collaborator

@jiandewang jiandewang left a comment

Choose a reason for hiding this comment

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

in UFS it only nuopc_cap so this change won't have any impact to us.

Copy link
Collaborator

@abozec abozec left a comment

Choose a reason for hiding this comment

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

COAPS approves. This change does not impact our set-up.

Copy link
Collaborator

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

This passes our regression test, and appears to be safe.

(Index capitalization in mask2dC[uv] may need a revision at some point, however,)

@sanAkel
Copy link
Collaborator Author

sanAkel commented Oct 28, 2023

This passes our regression test, and appears to be safe.

(Index capitalization in mask2dC[uv] may need a revision at some point, however,)

@marshallward Feel free to capitalize/ push an update. Anyway you'll have to update the branch.

@marshallward
Copy link
Collaborator

It's fine for now. I don't want to change the GEOS main commit at this stage.

@sanAkel
Copy link
Collaborator Author

sanAkel commented Oct 31, 2023

@gustavo-marques, @alperaltuntas would you please comment on this PR. Thanks!

@gustavo-marques
Copy link
Collaborator

Apologies for the delay. We approve this PR.

@sanAkel
Copy link
Collaborator Author

sanAkel commented Oct 31, 2023

Thanks @gustavo-marques.

@marshallward please update/rebase this branch, I can't do it myself via github - write permissions?
Thank you!

@marshallward
Copy link
Collaborator

Ah because it's out of date... you could fix this by merging main into your branch. But I can do it if you like.

@marshallward marshallward merged commit dbda49c into mom-ocean:main Oct 31, 2023
10 checks passed
@marshallward
Copy link
Collaborator

Requiring that branches be up-to-date feels incorrect, since we don't require partner branches to be using main, so I've disabled this requirement.

We can talk more at a future dev call if there are any potential problems with relaxing this rule.

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.

7 participants