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.

Sign up for GitHub

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

Jump to bottom

Further clean up and re-organize make_bcs #634

Merged
merged 34 commits into from
Dec 2, 2022
Merged

Further clean up and re-organize make_bcs #634

merged 34 commits into from
Dec 2, 2022

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Sep 9, 2022 •
edited by gmao-rreichle
Loading

One step in make_bcs cleanup project (https://github.com/GEOS-ESM/GEOSgcm_GridComp/projects/4):

  • Combines easeV1 and easeV2 modules into single module with generic interface
  • Combines mkSMAPTilesPara.F90 and mkSMAPTilesPara_v2.F90 into single program that creates EASE tiles.

PR is limited to bcs generation package and therefore trivially 0-diff for GCM and GEOSldas nightly tests.

PR is 0-diff for generation of bcs (confirmed by Biljana 30 Nov 2022).

PR also introduces a python version of make_bcs which remains under development.

@weiyuan-jiang weiyuan-jiang requested review from a team as code owners September 9, 2022 14:45
@weiyuan-jiang weiyuan-jiang changed the title combine easeV1 and easev2 Further clean up and re-organize make_bcs Sep 9, 2022
@gmao-rreichle gmao-rreichle marked this pull request as draft September 12, 2022 11:41
@gmao-rreichle gmao-rreichle added the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Sep 12, 2022
@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang, thanks for putting this together. Unfortunately, this PR has to wait until feature/borescan_snowalb_modis2 is merged into develop (#618). Merging this PR into feature/borescan_snowalb_modis2 now would make #618 too complex, and #618 has to be merged ASAP (after resolution of the outstanding questions).
One question: Do we still need the file mkSMAPTilesPara.F90? It looks like this file can be removed from the branch, but maybe I'm misunderstanding something.
I will review in more detail at a later time.

@weiyuan-jiang
Copy link
Contributor Author

@weiyuan-jiang, thanks for putting this together. Unfortunately, this PR has to wait until feature/borescan_snowalb_modis2 is merged into develop (#618). Merging this PR into feature/borescan_snowalb_modis2 now would make #618 too complex, and #618 has to be merged ASAP (after resolution of the outstanding questions).
One question: Do we still need the file mkSMAPTilesPara.F90? It looks like this file can be removed from the branch, but maybe I'm misunderstanding something.
I will review in more detail at a later time.

No, we don't need mkSMAPTilesPara.F90. This is PR is still in the process. I leave it there for comparison. Later on, the name mkSMAPTilesPara_v2.F90 will be changed to mkSMAPTilesPara.F90.

@weiyuan-jiang
Copy link
Contributor Author

It will be broke up into smaller PRs. Closed for now

@weiyuan-jiang
Copy link
Contributor Author

After discussion with @gmao-rreichle , we reopen this PR which will replace PR #639 . This PR will address comments in PR #639

@weiyuan-jiang weiyuan-jiang reopened this Sep 14, 2022
@weiyuan-jiang weiyuan-jiang mentioned this pull request Sep 14, 2022
Closed
weiyuan-jiang and others added 9 commits September 14, 2022 12:51
@weiyuan-jiang
use different PIs for easeV1 and easeV2 for zero-diff bcs
b8c939b
@weiyuan-jiang
add ease_get_params for cleanup
24d8748
@weiyuan-jiang
pass EASElable
0ec8f4a
@weiyuan-jiang
add choice for EASE grid
e0a5f68
@weiyuan-jiang
Merge branch 'feature/borescan_snowalb_modis2' into feature/wjiang/fu…
3297e05 
…rther_reorg_bcs
@weiyuan-jiang
typo
766dd19
@gmao-rreichle
Additional cleanup and documentation:
7a91c55 
- EASE_conv.F90
  - Added/edited comments
  - Fixed indentation
  - Cleaned up error messages
  - Replace “gridname” with “EASELabel”
- mkSMAPTilesPara_v2.F90 (and make_bcs)
  - For clarity, replaced “smap” with “ease” in variable names, print statements, comments, etc
@weiyuan-jiang
fix typo
d692b0c
@gmao-rreichle
Additional cleanup:
614a304 
- replaced ease_get_params() with simpler ease_extent() (EASE_conv.F90, mkSMAPTilesPara_v2.F90)
- renamed EVERSION to EASEVERSION for clarity (make_bcs)
@gmao-rreichle gmao-rreichle added documentation Improvements or additions to documentation 0 diff The changes in this pull request have verified to be zero-diff with the target branch. 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) cleanup labels Sep 22, 2022
@gmao-rreichle gmao-rreichle mentioned this pull request Nov 16, 2022
Merged
@gmao-rreichle gmao-rreichle linked an issue Nov 23, 2022 that may be closed by this pull request
debug mode exposed an error that exists in optimized mode #596
Closed
@gmao-rreichle gmao-rreichle removed a link to an issue Nov 23, 2022
debug mode exposed an error that exists in optimized mode #596
Closed
@sanAkel
Copy link
Contributor

sanAkel commented Nov 24, 2022 •
edited by gmao-rreichle
Loading

make_bcs should not make the same sym links over and again! It is superfluous!
See here for details.

Response by @gmao-rreichle added 14 Dec 2022: The links are only created once. But Sarith's old csh script includes three different blocks (one each for the lat/lon, EASE and cube-sphere cases) that duplicate the code that creates the links. We are working on a python version of this script that should be cleaner.

Base automatically changed from feature/borescan_snowalb_modis2 to develop November 29, 2022 19:45
@gmao-rreichle
Copy link
Contributor

FYI, I merged develop into this branch, which required resolving a conflict. Specifically, mk_CatchRestarts.F90 needed to be restored to an earlier version as in the following commit: 20655cb (from PR #618)

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin : I think this PR is now ready for final re-testing. (Note that develop has changed considerably since we last tested this PR two months ago.) The standard "nightly" GEOS GCM and LDAS tests are trivially 0-diff because this PR only changes the Raster directory, so I don't think the "nightly" tests will be needed. But please verify again that the bcs generation remains 0-diff. If you're using GEOSldas for this purpose, make sure to use the branch from GEOS-ESM/GEOSldas#586
Once we have reaffirmed that the bcs generation is 0-diff, I'll approve and we can forward it to Scott.

@biljanaorescanin
Copy link
Contributor

@gmao-rreichle @weiyuan-jiang fix you proposed yesterday worked.
Fix worked for CF cases that have multiple options for rst file as well due to the "gfile" name.
I've retested and all cases I tried are zero diff except EASE M25 that is correct now.

@biljanaorescanin biljanaorescanin marked this pull request as ready for review December 1, 2022 12:33
@biljanaorescanin biljanaorescanin requested a review from a team as a code owner December 1, 2022 12:33
@gmao-rreichle gmao-rreichle removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Dec 1, 2022
gmao-rreichle
gmao-rreichle previously approved these changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

@sdrabenh, this PR is ready for merging into develop. Please let us know if you have any questions.

gmao-rreichle
gmao-rreichle approved these changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

Re-approving after last-minute commit that added an exit message to make_bcs.py, which remains under development.

mathomp4
mathomp4 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@mathomp4 mathomp4 left a comment

Choose a reason for hiding this comment

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

CMake is good

@sdrabenh sdrabenh merged commit 86ede16 into develop Dec 2, 2022
@sdrabenh sdrabenh deleted the feature/wjiang/further_reorg_bcs branch December 2, 2022 16:34
gmao-rreichle added a commit to GEOS-ESM/GEOSldas that referenced this pull request Dec 6, 2022
@gmao-rreichle
use cleaned-up EASE grid tools (#586)
5a8dfa4 
Use cleaned-up make_bcs utilities (EASE grid tools) from GEOS-ESM/GEOSgcm_GridComp#634
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biljanaorescanin biljanaorescanin biljanaorescanin left review comments

@mathomp4 mathomp4 mathomp4 approved these changes

@sdrabenh sdrabenh sdrabenh approved these changes

@gmao-rreichle gmao-rreichle gmao-rreichle approved these changes

Assignees

@weiyuan-jiang weiyuan-jiang

Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) 0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@weiyuan-jiang @gmao-rreichle @biljanaorescanin @sanAkel @mathomp4 @sdrabenh