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

Further clean up and re-organize make_bcs #586

Merged
merged 16 commits into from
Dec 6, 2022

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Sep 28, 2022

replace LDAS_ EASE_conv.F90 with EASE_conv.F90 from Raster and clean up some calls related to EASE_conv

depends on GEOS-ESM/GEOSgcm_GridComp#634

@weiyuan-jiang
Copy link
Contributor Author

It is highly possible this PR would be non-zero diff 1) cell area is calculated 2) the box ur and ll are calculated now

@gmao-rreichle gmao-rreichle changed the title Feature/wjiang/further reorg bcs Further clean up and re-organize make_bcs Sep 29, 2022
@gmao-rreichle gmao-rreichle marked this pull request as draft September 29, 2022 16:07
@gmao-rreichle
Copy link
Contributor

Limited "nightly" test conducted by @weiyuan-jiang on 12-Oct-2022 after commit ef6fd43
Branch runs successfully again but fails the comparison for all tests that use the EASE tile space, as expected.
Postponed full verification of non-0-diff changes until after we have resolved the dependency on GEOS-ESM/GEOSgcm_GridComp#634

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Not 0-diff, infrastructure, cleanup, Contingent -- Do Not Approve

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Not 0-diff, infrastructure, cleanup, Contingent -- Do Not Approve

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Not 0-diff, infrastructure, cleanup, Contingent -- Do Not Approve

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin : I don't know if the current "develop" branch (prior to merging this PR) will now work with the updated GCM GC develop branch that includes GCM GC PR #634. It's possible we need the present GEOSldas PR or GEOSldas develop will no longer work. On the other hand, this PR is not 0-diff and we left the final testing open. We'll need to retest this PR as soon as possible.

@biljanaorescanin
Copy link
Contributor

Test summary attached:
Screen Shot 2022-12-02 at 4 28 11 PM

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin, @weiyuan-jiang, I went through Biljana's tests.
In the tilegrids.bin files, we have the following difference:
BASELINE: gridtype: 'EASEv2_M09'
CURRENT: gridtype: 'EASEv2-M09'
That is, we changed the underscore to a dash in the gridtype label that is archived in the tilegrids file. I don't remember if this was intentional. For reference, the tilegrids file from the Version 7 L4_SM product has an underscore in the gridtype label.
I think we intentionally changed the source code so that both strings will work in GEOSldas runs.
@weiyuan-jiang, do you remember anything about this? Where exactly does this string come from when it's dumped into the tilegrids bin file?

The rest of the differences checks out ok:
The HISTORY and rst differences in the "assim" comparisons are within roundoff and as expected.
The differences in the tilecoord.bin files are roundoff in the field "area" and as expected.
In the tilegrids.bin files there are roundoff differences in the ll_lat, ll_lon, ur_lat, ur_lon, dlon, and dlat numbers, as expected.

@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented Dec 5, 2022

@gmao-rreichle @biljanaorescanin I think we have changed the name from "_Mxx" to "-Mxx" ...but let me double check. I think we make the names consistent "_Mxx"

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Not 0-diff, infrastructure, cleanup, Contingent -- Do Not Approve

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Not 0-diff, infrastructure, cleanup, Contingent -- Do Not Approve

@gmao-rreichle
Copy link
Contributor

@gmao-rreichle @biljanaorescanin I think we have changed the name from "_Mxx" to "-Mxx" ...but let me double check. I think we make the names consistent "_Mxx"

@weiyuan-jiang, @biljanaorescanin: I looked into this a bit more and am now quite confident of the following:

  1. I think the primary objective was to accommodate both "-Mxx" and "_Mxx" in existing tilegrids files when they are processed in the source code (for backward compatibility). This worked fine.

  2. We intentionally changed the "gridname" in the tilegrids file to match what is in the bcs *.til file, which is really the source of everything. I remember we opted to stick with the less desirable and less consistent "-Mxx" string because this was already used in bcs going back to at least NLv3.

  3. Note that the original "tilegrids" file in Matt's input directory:
    /discover/nobackup/mathomp4/LDAS_Restarts/NGHTLY_TST/model/input/restart/m36_global_regression/output/SMAP_EASEv2_M36_GLOBAL/rc_out/m36_global_regression.ldas_tilegrids.bin
    uses "_Mxx". When Biljana tested the branch, "gridname" was taken from the bcs file via subroutine LDAS_read_til_file() and inserted into the tilegrids file via subroutine LDAS_create_grid_g() as intended, which correctly reproduces the desired "-Mxx" string in the tilegrids file in the branch. This is an intentional change and should fail the comparison with Matt's present BASELINE experiment. Weiyuan's most recent commit overrides this, so I reverted back to the branch as tested by
    Biljana.

  4. Once this is merged, the most of the LDAS tests will fail the comparison and need to be re-baselined. I'll go ahead and merge. With any luck, @mathomp4 can re-baseline tomorrow.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review December 5, 2022 22:50
@gmao-rreichle
Copy link
Contributor

@GEOS-ESM/cmake-team : Can you please approve this PR when you get a chance?
cc: @weiyuan-jiang @biljanaorescanin

@gmao-rreichle gmao-rreichle merged commit 5a8dfa4 into develop Dec 6, 2022
@gmao-rreichle gmao-rreichle deleted the feature/wjiang/further_reorg_bcs branch December 6, 2022 02:18
@gmao-rreichle
Copy link
Contributor

@mathomp4 : This is interesting... I don't think this was formally approved by the CMake team but github still let me merge it, so I went ahead and merged, if only to see what would happen.
I merged the PR just after tonight's nightly LDAS tests had cloned the repo for the GNU tests. So chances are tonight's (Monday's) tests will pass, and tomorrow's (Tuesday's) tests will (mostly) fail as expected.

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

Successfully merging this pull request may close these issues.

3 participants