Skip to content

Conversation

@weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented May 8, 2025

Types of change(s)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)
  • Refactor (no functional changes, no api changes)

Checklist

  • Tested this change with a run of GEOSgcm
  • Ran the Unit Tests (make tests)
  • Successfully 0-diff tested with make_bcs
  • Successfully 0-diff tested for GEOSldas

Description

Add EASEGridFactory so the Regridder can use EASEGrid.

Related Issue

Dependencies/Contingencies

GEOS-ESM/GEOSgcm_GridComp#1112

@weiyuan-jiang weiyuan-jiang added 🎁 New Feature This is a new feature 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. labels May 8, 2025
@weiyuan-jiang weiyuan-jiang changed the title initial check in Add EASE grid May 19, 2025
@weiyuan-jiang weiyuan-jiang marked this pull request as ready for review May 28, 2025 16:50
@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner May 28, 2025 16:50
@mathomp4
Copy link
Member

@weiyuan-jiang It looks like there is some ambiguity in some GEOS code:

[ 83%] Building Fortran object src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkEASETilesParam.x.dir/mkEASETilesParam.F90.o
/root/project/GEOSgcm/src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/mkCatchParam.F90:193:23:

  193 |       call ease_extent(Gridname, nc_ease, nr_ease )
      |                       1
Error: Name 'ease_extent' at (1) is an ambiguous reference to 'ease_extent' from module 'mapl_easeconvmod'
make[2]: *** [src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkCatchParam.x.dir/build.make:75: src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkCatchParam.x.dir/mkCatchParam.F90.o] Error 1

Will this MAPL PR need to go with a GEOSgcm_GridComp PR?

@weiyuan-jiang
Copy link
Contributor Author

@weiyuan-jiang It looks like there is some ambiguity in some GEOS code:

[ 83%] Building Fortran object src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkEASETilesParam.x.dir/mkEASETilesParam.F90.o
/root/project/GEOSgcm/src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/mkCatchParam.F90:193:23:

  193 |       call ease_extent(Gridname, nc_ease, nr_ease )
      |                       1
Error: Name 'ease_extent' at (1) is an ambiguous reference to 'ease_extent' from module 'mapl_easeconvmod'
make[2]: *** [src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkCatchParam.x.dir/build.make:75: src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/makebcs/CMakeFiles/mkCatchParam.x.dir/mkCatchParam.F90.o] Error 1

Will this MAPL PR need to go with a GEOSgcm_GridComp PR?

I moved a file form GEOSgcm_gridComp to MAPL. So this PR should go with the PR there

@mathomp4 mathomp4 added the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jun 2, 2025
@mathomp4
Copy link
Member

mathomp4 commented Jun 2, 2025

@weiyuan-jiang Okay. We'll probably need to do this in a combined fashion. I might make a release of MAPL before this. Then we can work with Scott on getting a MAPL/GEOSgcm_GridComp combo in.

@mathomp4
Copy link
Member

mathomp4 commented Jun 2, 2025

Oh wait. I'm dumb. You moved it here so that means we just need a new release of MAPL.

@mathomp4 mathomp4 removed the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jun 2, 2025
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

Lots of simple (but tedious) changes requested to bring the EASEConv module up to MAPL standards.

weiyuan-jiang and others added 2 commits June 2, 2025 15:56
Co-authored-by: Tom Clune <thomas.l.clune@nasa.gov>
Co-authored-by: Tom Clune <thomas.l.clune@nasa.gov>
tclune
tclune previously approved these changes Jun 12, 2025
@weiyuan-jiang weiyuan-jiang added the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jun 12, 2025
…e-space changes to facilitate commenting on Github.com (MAPL_EASEConversion.F90, MAPL_EASEGridFactory.F90, NCIO.F90)
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.

@weiyuan-jiang @mathomp4 @tclune : I made a couple of minor changes (cbb0e5b) and added a few comments/questions below.
Also, the "Conversation" tab on the PR only states that the PR was tested with GEOSgcm and some unit tests. Before we can merge the PR, we need to test make_bcs. Has this been 0-diff tested for make_bcs? If so, please state this the Conversation. If not, @biljanaorescanin can help
Please take a look and let me know if you have any questions.

@biljanaorescanin
Copy link

@gmao-rreichle PR was tested for BCS package creation for v12 EASE M09 and v13 EASE M36 and it is zero diff.
To run make_bcs needs 3 branches:
MAPL | (t) v2.56.0 (DH) | (b) feature/wjiang/ease_grid
GEOSldas_GridComp | (b) develop (DH, 4c7de26) | (b) feature/wjiang/ease_grid
GEOSgcm_GridComp | (b) develop (DH, 85b060ab) | (b) feature/wjiang/ease_grid

@gmao-rreichle
Copy link
Contributor

@gmao-rreichle PR was tested for BCS package creation for v12 EASE M09 and v13 EASE M36 and it is zero diff. To run make_bcs needs 3 branches:
MAPL | (t) v2.56.0 (DH) | (b) feature/wjiang/ease_grid
GEOSldas_GridComp | (b) develop (DH, 4c7de26) | (b) feature/wjiang/ease_grid
GEOSgcm_GridComp | (b) develop (DH, 85b060ab) | (b) feature/wjiang/ease_grid

Thanks, @biljanaorescanin, for the test results. Did you run make_bcs from a build of GEOSgcm or GEOSldas?

@weiyuan-jiang: I don't see a PR for GEOSldas_GridComp. It's probably best to switch to the new EASEGridFactory now, since you already have the branch. Please create the GEOSldas_GridComp PR and the corresponding GEOSldas PR (with the components.yaml update, which can be the MAPL branch for now). Thanks!

@biljanaorescanin
Copy link

Thanks, @biljanaorescanin, for the test results. Did you run make_bcs from a build of GEOSgcm or GEOSldas?

@gmao-rreichle I've tested from GEOSldas. Do you need me to do the same from GEOSgcm?

@weiyuan-jiang
Copy link
Contributor Author

Thanks, @biljanaorescanin, for the test results. Did you run make_bcs from a build of GEOSgcm or GEOSldas?

@gmao-rreichle I've tested from GEOSldas. Do you need me to do the same from GEOSgcm?

Do you test bcs generated by these PRs? That is important

@biljanaorescanin
Copy link

Do you test bcs generated by these PRs? That is important

Yes, few comments up I say I used those 3 branches for bcs creation.
I can share experiment outputs on chat if you need to see it.

This was my testing clone:
https://github.com/GEOS-ESM/GEOSldas/blob/testing/borescan_ease_factory/components.yaml

@gmao-rreichle
Copy link
Contributor

Thanks, @biljanaorescanin, for the test results. Did you run make_bcs from a build of GEOSgcm or GEOSldas?

@gmao-rreichle I've tested from GEOSldas. Do you need me to do the same from GEOSgcm?

No, make_bcs should be identical when run from either App. I mostly wanted to know if you worked within GEOSldas, and how the GEOSldas_GridComp branch fits into all of this. This has now been addressed with new PRs for GEOSldas and GEOSldas_GridComp. I added a new test requirement to the intro comment above (0-diff for GEOSldas).

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.

@mathomp4, @weiyuan-jiang : The new MAPL EASE Grid Factory has been successfully 0-diff tested with GEOSldas and GEOSgcm. The PR is ready to be merged as far as I'm concerned.
To use the new functionality in GEOSldas, we'll need a new MAPL release. Both GEOSldas and GEOSgcm are on MAPL v2.56, so the new MAPL release should work ok for both.
I'm a bit unsure about where we are with GEOSadas (which now includes the LDAS in its build). I know Sara was sorting out MAPL versions for ADAS and LDAS recently, so whatever the issue was then might still be an issue now. But we can sort this out later.
cc: @tclune @biljanaorescanin

@gmao-rreichle gmao-rreichle removed the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jun 17, 2025
@mathomp4
Copy link
Member

@gmao-rreichle Sounds good. I'll try and get a new MAPL out for you soon.

@mathomp4 mathomp4 merged commit 1d7f25e into develop Jun 17, 2025
30 checks passed
@mathomp4 mathomp4 deleted the feature/wjiang/ease_grid branch June 17, 2025 17:43
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. 🎁 New Feature This is a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants