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

Feature/yury/s2s3 merge #293

Merged
merged 14 commits into from
Jun 12, 2020
Merged

Feature/yury/s2s3 merge #293

merged 14 commits into from
Jun 12, 2020

Conversation

yvikhlya
Copy link

@yvikhlya yvikhlya commented Jun 5, 2020

This PR has a number of zero diff updates from S2S3 tag in CVS. It depends on GEOS-ESM/MAPL#405 and GEOS-ESM/MOM5#14. @sanAkel please review GEOS_SurfGridComp.F90 and GEOS_OpenWaterGridComp.F90. Updates in GEOS_GcmGridComp.F90, GEOS_OgcmGridComp.F90 and GuestOcean_GridComp.F90 affect dual ocean mode. Updates to MOM_GEOS5PlugMod.F90 fix export of mixed layer depth from MOM5 and record restart in dual ocean mode.

sdrabenh and others added 7 commits May 21, 2020 10:19
…s of salt

flux due to SSS relaxation to SSS_MIN, which prevents model from crashing due
to low salinity.
According to Atanas, this update fixes a bug to set the proper mode to write
to RAM in dual ocean mode for OGCM and children.
@yvikhlya yvikhlya added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) labels Jun 5, 2020
@yvikhlya yvikhlya requested review from a team as code owners June 5, 2020 16:55
@yvikhlya yvikhlya self-assigned this Jun 5, 2020
@yvikhlya yvikhlya added the communication/coordination Communication & coordination across repositories label Jun 5, 2020
@yvikhlya
Copy link
Author

yvikhlya commented Jun 5, 2020

@mathomp4 By the way, I just noticed that Externals.cfg has geos/v1.0.1 tag for MOM5, which does not have the required export, and this will not compile. Should I change it to geos5 branch?

@mathomp4
Copy link
Member

mathomp4 commented Jun 5, 2020

@mathomp4 By the way, I just noticed that Externals.cfg has geos/v1.0.1 tag for MOM5, which does not have the required export, and this will not compile. Should I change it to geos5 branch?

No. The correct thing would be to make a new release of MOM5 (geos/v1.0.2) and then @sdrabenh can update the Externals/components.yaml to that.

There are some changes in the geos5 branch that haven't been released yet:

GEOS-ESM/MOM5@geos/v1.0.1...geos5

@mathomp4
Copy link
Member

mathomp4 commented Jun 5, 2020

I could make a new release (or you could) and then you could edit the Externals.cfg at the same time. (Note, the components.yaml in GEOSgcm would need to change at the same time)

@atrayano
Copy link
Contributor

atrayano commented Jun 5, 2020 via email

@yvikhlya
Copy link
Author

yvikhlya commented Jun 5, 2020

I could make a new release (or you could) and then you could edit the Externals.cfg at the same time. (Note, the components.yaml in GEOSgcm would need to change at the same time)

Can I ask you to handle the release and dependency fix? You know the rules better than me.

@atrayano
Copy link
Contributor

atrayano commented Jun 5, 2020 via email

@yvikhlya
Copy link
Author

yvikhlya commented Jun 5, 2020

I actually meant @mathomp4, could you handle the release and dependency updates in config files?

zhaobin74
zhaobin74 previously approved these changes Jun 5, 2020
Copy link
Contributor

@zhaobin74 zhaobin74 left a comment

Choose a reason for hiding this comment

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

The SST relaxation part looks good.

@mathomp4
Copy link
Member

mathomp4 commented Jun 5, 2020

I have released a new version of MOM5 with all the bug fixes since the last. I've updated the Externals.cfg here.

I also made GEOS-ESM/GEOSgcm#153 which updates components.yaml to geos/v1.0.2

@yvikhlya
Copy link
Author

yvikhlya commented Jun 5, 2020

@mathomp4 Thanks Matt. It still needs the dependency on GEOS-ESM/MAPL#405 to be satisfied before it can build.

sanAkel
sanAkel previously requested changes Jun 7, 2020
Copy link
Contributor

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

@yvikhlya
Just 3 comments/requests:

1. Are you all using a particular notation for dual ocean variables,
e.g., in GEOSogcm_GridComp/GEOS_OgcmGridComp.F90
Things like:

  • DST_NAME = (/'FRACICEd'/), &
  • SRC_ID = SEAICEd,
    with a d in the end?
    If so, please state that explicitly in the preamble. That will help a long ways!

2.1 In GEOSogcm_GridComp/GEOSocean_GridComp/GuestOcean_GridComp/GuestOcean_GridComp.F90

The saltrestore, will not work with MOM6 since we are not exporting DH, hence DH(:,:,1) isn't available. But then I see no reason, why this block cannot be moved into MOM5_plug.

FYI: MOM6 salt flux restore https://github.com/sanAkel/MOM6/blob/17339577cec87bace340f46515155a80eaaf20f1/config_src/coupled_driver/MOM_surface_forcing.F90#L346

For now, would you please move this block from Guest ocean to MOM5 plug. Thanks!

2.2 In calculation of DEL_TMP, restoring temperature difference, please put in a comment with what the value of -0.054 is with its units. @zhaobin74

Question
What the addition in GEOS_GcmGridComp.F90 do??
Was it needed by MAPL2? Or something else?

@yvikhlya
Copy link
Author

yvikhlya commented Jun 10, 2020

@mathomp4 @sdrabenh All dependencies are fixed now. In order to pass circleci tests components.yaml should be updated to use geos/v1.0.2 of mom, a tag for MAPL master branch should be issued and components.yaml updated accordingly.

zhaobin74
zhaobin74 previously approved these changes Jun 10, 2020
@mathomp4
Copy link
Member

@mathomp4 @sdrabenh All dependencies are fixed now. In order to pass circleci tests components.yaml should be updated to use geos/v1.0.2 of mom, a tag for MAPL master branch should be issued and components.yaml updated accordingly.

@yvikhlya It's possible you might have to push one more change. One of the MAPL calls that @atrayano changed for you might need you to use keyword arguments so one of them can become optional.

@yvikhlya
Copy link
Author

@mathomp4 @sdrabenh All dependencies are fixed now. In order to pass circleci tests components.yaml should be updated to use geos/v1.0.2 of mom, a tag for MAPL master branch should be issued and components.yaml updated accordingly.

@yvikhlya It's possible you might have to push one more change. One of the MAPL calls that @atrayano changed for you might need you to use keyword arguments so one of them can become optional.

@mathomp4 @atrayano Ok, let me know what I need to update.

@atrayano
Copy link
Contributor

@yvikhlya I already had sent you a Team's chat message with instructions what you need to change. Here it is again:
Please add "MODE=" to this line in the plug

doRecord = MAPL_RecordAlarmIsRinging(MAPL, MAPL_Write2Disk, RC=status)

to

doRecord = MAPL_RecordAlarmIsRinging(MAPL, MODE=MAPL_Write2Disk, RC=status)

@mathomp4
Copy link
Member

@yvikhlya Once you can confirm things work with MAPL master and the edit @atrayano has suggested, I can release MAPL 2.1.5

@yvikhlya yvikhlya dismissed stale reviews from zhaobin74 and sanAkel via a9a2d0d June 11, 2020 13:53
@yvikhlya
Copy link
Author

@mathomp4 @atrayano MODE=MAPL_Write2Disk works and I pushed a commit.

@atrayano
Copy link
Contributor

@yvikhlya Thanks for testing it! The real question is if the version of MAPL on the master branch works too, so @mathomp4 can proceed with the release of v2.1.5

@yvikhlya
Copy link
Author

@yvikhlya Thanks for testing it! The real question is if the version of MAPL on the master branch works too, so @mathomp4 can proceed with the release of v2.1.5

I've tested it with MAPL master.

@mathomp4
Copy link
Member

MAPL 2.1.5 has been released.

@yvikhlya
Copy link
Author

MAPL 2.1.5 has been released.

Will you update components.yaml?

@mathomp4
Copy link
Member

MAPL 2.1.5 has been released.

Will you update components.yaml?

Done! I've updated the branch on GEOSgcm to have MAPL 2.1.5 in all the files.

@mathomp4
Copy link
Member

Note: CircleCI will fail because the CI isn't as smart as it should be. The reason is that it clones GEOSgcm first. But it doesn't know it needs a different branch of the fixture to get the CI to pass.

If the GEOSgcm was updated to have MAPL 2.1.5 and mom 1.0.2, it would build and pass.

@sdrabenh
Copy link
Collaborator

@mathomp4 correct, but for testing, the branch should be to be up-to-date with develop

@mathomp4
Copy link
Member

@mathomp4 correct, but for testing, the branch should be to be up-to-date with develop

Yes. For testing, you'll want to first do:

git checkout feature/yury/s2s3-merge

after cloning the fixture. Then you can do a mepo init && mepo clone.

@sdrabenh
Copy link
Collaborator

@yvikhlya my tests do not show this PR is 0-diff. In both amip and replay mode, openwater_internal_checkpoint has different values for the SSKINW field ...

Date Time Level Gridsize Miss Diff : S Z Max_Absdiff Max_Reldiff : Parameter name 7 : 2016-12-04 21:00:00 0 99389 0 99389 : F F 25.000 0.83333 : SSKINW 1 of 12 records differ cdo diffn: Processed 24 variables over 2 timesteps [0.26s 19MB].

@yvikhlya
Copy link
Author

yvikhlya commented Jun 11, 2020

@sdrabenh Just a single restart? Let me check this again. This may have have something to do with SS calculation which I changed. Do you run with use_skin_layer on of off?

I cast @sanAkel here too.

Yury Vikhliaev added 2 commits June 11, 2020 16:21
…diff with

use_skin_layer=1 and affected other computations in coupled mode.
@yvikhlya
Copy link
Author

@sdrabenh I amended changes to GEOS_OpenWaterGridComp.F90 which caused non-zero diff under condition when use_skin_layer=1. We will deal with SSKINW computation later in separate pull request.

@yvikhlya yvikhlya removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Jun 12, 2020
@sdrabenh sdrabenh merged commit f289046 into develop Jun 12, 2020
@sdrabenh sdrabenh deleted the feature/yury/s2s3-merge branch June 12, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. communication/coordination Communication & coordination across repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants