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

cleanup and reorganization of files in ./Raster (make_bcs) #707

Merged
merged 16 commits into from
Mar 6, 2023
Merged

cleanup and reorganization of files in ./Raster (make_bcs) #707

merged 16 commits into from
Mar 6, 2023

Conversation

gmao-rreichle
Copy link
Contributor

@gmao-rreichle gmao-rreichle commented Feb 21, 2023 •
edited
Loading

Intended to replace #706.

  • new directory "./makebcs/" contains scripts and programs to create bcs
  • new directory "./preproc/" captures some scripts and programs to create inputs to make_bcs (INCOMPLETE!)
  • new directory "./misc/" contains potentially still useful debugging and plotting scripts for make_bcs

UPDATED 4 March 2023:

  • trivially 0-diff for GCM (PR touches only CatchCN and Raster/make_bcs files)
  • make_bcs successfully 0-diff tested by @biljanaorescanin
  • left for another time: remove duplication of functionality in ./makebcs/CubedSphere_GridMod.F90 (should be using ESMF)
  • I'm happy with the PR as it stands right now but since it's my own I cannot approve it. @biljanaorescanin, @weiyuan-jiang, @mathomp4, please double-check and approve for the respective teams (Land, Surface Preproc, etc) and move it on to @sdrabenh's desk, or let me know if you have any questions/concerns. Thanks!

@gmao-rreichle
cleanup and reorganization of files in Raster
ab03f96 
- new directory "./makebcs/" contains scripts and programs to create bcs
- new directory ".preproc/" contains scripts and program to create *input* to make_bcs
- new directory "./misc/" contains potentially still useful debugging and plotting scripts for 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. Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) cleanup labels Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Feb 21, 2023
@gmao-rreichle
Copy link
Contributor Author

@biljanaorescanin, @weiyuan-jiang,

I started over with #706 here for two reasons. One was that #706 somehow ended up adding "execute" permission to F90 files, presumably because the files were first deleted and then added, rather than moved via "git mv ...". The branch associated with this new PR (#707) used "git mv..." to move the files. The other reason was that I wanted to add a new ./misc/ directory that captures some potentially still useful debugging and plotting tools.

There are a few outstanding issues:

  • I did not (yet) add CMakeLists.txt files into the ./preproc/ subdirectories. While it may be good to compile any F90 code in these directories during the build, I'm not convinced that we want to clutter up the build's ./bin directory with the corresponding ("preproc") executables. Something to discuss.
  • Do we need the file ./makebcs/compile_mk_runoff ?? Shouldn't this be done through CMakeLists.txt?
  • ./makebcs/date_time_util.F90 and ./makebcs/leap_year.F90 are predecessors of LDAS_DateTimeMod.F90 in the GEOSldas GridComp. I'm not sure how best to resolve the duplication.
  • What does ./makebcs/CubedSphere_GridMod.F90 do that isn't in MAPL? Can't we use MAPL tools instead?
  • In the CMakeLists.txt file(s), what does esma_set_this (OVERRIDE [dirname?]) do? This might need fixing.
  • Do we need (all of?) Raster.h? It defines its own pi, among other things, which unfortunately differs from MAPL_PI_R8.

@gmao-rreichle gmao-rreichle mentioned this pull request Feb 21, 2023
Closed
@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions

This comment was marked as duplicate.

Sign in to view
@github-actions

This comment was marked as duplicate.

Sign in to view
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

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

@github-actions
Copy link

github-actions bot commented Mar 1, 2023

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

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

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

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

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

@gmao-rreichle gmao-rreichle mentioned this pull request Mar 2, 2023
Merged
@biljanaorescanin
Copy link
Contributor

PR was tested for C720 NLv3 O2, C180 ICA O2, NLv5 M09, C12 NLv3 T4MOM6 all were zero diff.

@biljanaorescanin
Copy link
Contributor

@gmao-rreichle should I resolve conversations or will you address that?

@biljanaorescanin biljanaorescanin marked this pull request as ready for review March 4, 2023 13:51
@biljanaorescanin biljanaorescanin requested review from a team as code owners March 4, 2023 13:51
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

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

@gmao-rreichle gmao-rreichle removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Mar 4, 2023
mathomp4
mathomp4 approved these changes Mar 6, 2023
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.

Super approve per @gmao-rreichle

@sdrabenh sdrabenh merged commit 9cfd09c into develop Mar 6, 2023
@sdrabenh sdrabenh deleted the feature/rreichle/make_bcs_cleanup branch March 6, 2023 18:51
@gmao-rreichle gmao-rreichle mentioned this pull request Mar 6, 2023
Merged
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

@weiyuan-jiang weiyuan-jiang weiyuan-jiang approved these changes

@sdrabenh sdrabenh sdrabenh approved these changes

Assignees
No one assigned
Labels
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
5 participants
@gmao-rreichle @biljanaorescanin @mathomp4 @weiyuan-jiang @sdrabenh