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

Cleanup of make_bcs tools that generate raster and tile files #763

Merged
merged 19 commits into from
Jul 17, 2023

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented May 30, 2023

Cleanup of make_bcs tools that generate raster and tile files.

  • Addresses EASE grid naming convention GEOSldas#605.

  • Major cleanup of mkEASETilesParam.F90, which was necessary to understand what the program is doing and how to possibly make it work for the high-resolution M01 (1-km) EASE grid.
    During cleanup, a couple of concerning issues were discovered:

    • The min/max lat/lon values for each tile in the catchment.def file seem to be associated with the EASE grid cell and not the desired lat/lon boundaries of the tile.
    • The Pfafstetter (catchment) ID assigned to a tile appears to be that of the raster grid cell that is the last to contribute to the tile; it does not appear to be that of the dominant catchment within the EASE grid cell.
  • Some cleanup of mkLandRaster.F90.

Make sure to hide white-space changes when viewing differences.

@github-actions
Copy link

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@github-actions
Copy link

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@biljanaorescanin biljanaorescanin added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup labels Jun 15, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Jun 15, 2023
@GEOS-ESM GEOS-ESM deleted a comment from github-actions bot Jun 15, 2023
 - rewrote some code blocks for clarity and efficiency
 - removed obsolete variable declarations
 - removed obsolete use statements
 - added "only:" qualifier to some use statement
 - commented out obsolete code blocks
 - renamed some variables for clarity
 - removed repetition of identical operations
 - added comments
 - white-space changes for improved readability
…on of data in GEOS5_10arcsec_mask.nc (mkEASETilesParam.F90)
- cleaned-up 1-dim indexing for EASE and raster grid cells
- renamed loop counter variables for clarity
- cleaned up hardwired target for global mean elevation
- edited comments for clarity
- removed obsolete code
@gmao-rreichle
Copy link
Contributor

gmao-rreichle commented Jun 19, 2023

@weiyuan-jiang, @biljanaorescanin: I ended up in the rabbit hole of cleaning up mkEASETilesParam.F90. Here's a summary of where things stand right now:

  • With the latest commit dae09d9, I successfully reproduced the *.rst, *.til, and catchment.def file in the "legacy" bcs for EASEv2_M36 (with the expected header and white-space differences).
  • I don't think that the science is quite what it should be and what we all thought it was. This isn't something we want to change in this PR, which is meant to be 0-diff for bcs. But going forward, we may need a new bcs version that fixes these issues. This may or may not apply to the cube-sphere bcs. Specifically:
    • The min/max lat/lon values for each tile in the catchment.def file seem to be associated with the EASE grid cell and not the desired lat/lon boundaries of the tile.
    • The Pfafstetter (catchment) ID assigned to a tile appears to be that of the raster grid cell that is the last to contribute to the tile; it does not appear to be that of the dominant catchment within the EASE grid cell.
  • Based on my now much-improved understanding of the "geometry" files, I think I can confirm that the *.rst file is indeed what is needed to improve the mapping of the MODIS snow albedo into tile space; but I forget where things stand with enhancements for MODIS-based snow albedo over land #687.
  • During testing, it would have been helpful to have the EASEv2_M36 output from the develop branch in the ./bcs_shared/fvInput/ directory. @biljanaorescanin, please add this case. We'll then also need to verify for the EASE case that
    • all files (e.g., catchment.def, job script file, etc) end up in the right places
    • all print statements are captured in the log file(s)
  • The make_bcs *.j script only works when the job is submitted by the make_bcs script, because MAKE_BCS_INPUT_DIR is not set in the job script. That is, sbatch *.j does not work. I'm not sure if this is intentional and how this looks in the python version of make_bcs. (For testing, I used make_bcs and not make_bcs.py.)
  • I'm not sure if the adjustment of the land tile elevation properly accounts for lake and landice surfaces. This requires further examination.
  • I did not clean up the existing cavalier use of single- and double-precision variables. This will need further work.
  • I could add more documentation about the algorithm into the source code to make it easier to understand but I have to return to other work now. Maybe I can get back to adding more comments later.

cc: @rdkoster

@biljanaorescanin
Copy link
Contributor

I've moved one set of EASEv2_M36 to the area. I've run test with both make_bcs and make_bcs.py and result is the same between the two and is zero diff to what we have on develop.

During testing, it would have been helpful to have the EASEv2_M36 output from the develop branch in the ./bcs_shared/fvInput/ directory. @biljanaorescanin, please add this case. We'll then also need to verify for the EASE case that

  • all files (e.g., catchment.def, job script file, etc) end up in the right places
  • all print statements are captured in the log file(s)

@gmao-rreichle gmao-rreichle changed the title clean up make_bcs Cleanup of make_bcs tools that generate raster and tile files Jun 27, 2023
@gmao-rreichle
Copy link
Contributor

Update on earlier comments:

I don't think that the science is quite what it should be and what we all thought it was. [...]

  • The min/max lat/lon values for each tile in the catchment.def file seem to be associated with the EASE grid cell and not the desired lat/lon boundaries of the tile.
  • The Pfafstetter (catchment) ID assigned to a tile appears to be that of the raster grid cell that is the last to contribute to the tile; it does not appear to be that of the dominant catchment within the EASE grid cell.

The latter was confirmed by analyzing existing catchment.def and *.rst files. Going forward, a fix and new EASE bcs version will be needed under a new PR. It remains to be seen if the issues apply to the cube-sphere bcs.

  • Based on my now much-improved understanding of the "geometry" files, I think I can confirm that the *.rst file is indeed what is needed to improve the mapping of the MODIS snow albedo into tile space; but I forget where things stand with use tile_id for snow albedo #687.

Work is continuing on #687

  • [...] have the EASEv2_M36 output from the develop branch in the ./bcs_shared/fvInput/ directory.
    • all files (e.g., catchment.def, job script file, etc) end up in the right places
    • all print statements are captured in the log file(s)

Resolved.

  • The make_bcs *.j script only works when the job is submitted by the make_bcs script, because MAKE_BCS_INPUT_DIR is not set in the job script. That is, sbatch *.j does not work. I'm not sure if this is intentional and how this looks in the python version of make_bcs. (For testing, I used make_bcs and not make_bcs.py.)

Ok for make_bcs.py. Will not be fixed for make_bcs because csh script is becoming obsolete.

  • I'm not sure if the adjustment of the land tile elevation properly accounts for lake and landice surfaces. This requires further examination.

Still requires further examination. If there is an issue, it would not be fixed as part of this PR. Going forward, a fix and new bcs version may be needed under a new PR.

  • I did not clean up the existing cavalier use of single- and double-precision variables. This will need further work.

Mixed mode arithmetic is unfortunate but (probably) cannot be fixed without non-0-diff changes. Going forward, a fix and new bcs version may be needed under a new PR.

@gmao-rreichle gmao-rreichle linked an issue Jun 29, 2023 that may be closed by this pull request
@gmao-rreichle gmao-rreichle added the 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label Jul 5, 2023
@biljanaorescanin
Copy link
Contributor

This PR only affects boundary conditions creation package; for AGCM it is zero diff trivial.
PR was tested for C180 and M36 boundary conditions and its zero diff to what is on develop.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review July 12, 2023 13:21
@gmao-rreichle gmao-rreichle requested review from a team as code owners July 12, 2023 13:21
@gmao-rreichle
Copy link
Contributor

@mathomp4: I approved the PR but for reasons that remain a mystery to me, my approval does not count towards approval by @GEOS-ESM/surface-preproc-team. The python changes are trivial. Please super-approve when you get a chance.

@biljanaorescanin
Copy link
Contributor

@gmao-rreichle I can approve for surface-preproc-team

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.

Approve for @GEOS-ESM/python-transition-team

@mathomp4
Copy link
Member

@mathomp4: I approved the PR but for reasons that remain a mystery to me, my approval does not count towards approval by @GEOS-ESM/surface-preproc-team. The python changes are trivial. Please super-approve when you get a chance.

I'm still confused by this.

So I did a couple things. one, I removed you from the surface-preproc-team and then re-added you. Then I made you a Maintainer of it. Github can't say you don't belong now!

@sdrabenh sdrabenh merged commit fc141b3 into develop Jul 17, 2023
9 checks passed
@sdrabenh sdrabenh deleted the feature/wjiang/clean_up_make_bcs branch July 17, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EASE grid naming convention
5 participants