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

bug fix: PEATCLSM water table depth #593

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

gmao-rreichle
Copy link
Contributor

The water table depth (WATERTABLED) export variable ("zbar" in Catchment) that was introduced with PEATCLSM (#524) produces inappropriate values for mineral tiles.

This PR restricts the calculation of the WATERTABLED and FSWCHANGE exports to PEATCLSM tiles and assigns MAPL_UNDEF elsewhere. The PR also renames these exports for clarity and consistency:

WATERTABLED --> PEATCLSM_WATERLEVEL
FSWCHANGE   --> PEATCLSM_FSWCHANGE

Moreover, the sign convention for PEATCLSM_WATERLEVEL was flipped from that of WATERTABLED so that now water level is negative below ground and positive above ground.

One open question (probably for @weiyuan-jiang or @mathomp4):
The above PEATCLSM exports are calculated in tile space and remapped to the atmospheric grid via MAPL Location Stream, along with all other tile-space land exports. Since PEATCLSM tiles are still "land" tiles (i.e., type 100) and are identified only by their uniquely large porosity (POROS) values, the question is how no-data-values (MAPL_UNDEF) are treated in the remapping from tile to grid. The desired behavior is to ignore MAPL_UNDEF in the tile2grid operation. That is, if a given (say, cube-sphere) grid cell includes two tiles, one mineral and one PEATCLSM, then the average of PEATCLSM_WATERLEVEL for the cube-sphere grid cell in question should be that of the PEATCLSM tile, rather than MAPL_UNDEF. If this is not the default behavior, are there options in MAPL Location Stream to make this happen?

Since the PR only touches exports that are only used for the forthcoming SMAP L4_SM version, it should be trivially 0-diff for the GCM. Given the extent of the edits, though, I chose the regular 0-diff label to be safe.

There is some urgency to this bug fix because it is needed for L4_SM Version 7 production, which is estimated to start later this month.

A matching GEOSldas PR will be created.

The revised variable names and documentation are not set in stone. Feel free to comment if you have any questions or suggestions.

cc: @gmao-jkolassa @rdkoster @sdrabenh @biljanaorescanin @mbechtold @gmao-qliu

…ace water output:

- renamed WATERTABLED --> PEATCLSM_WATERLEVEL
- renamed FSWCHANGE   --> PEATCLSM_FSWCHANGE
- restricted above exports to PEATCLSM tiles
- flipped sign convention for PEATCLSM_WATERLEVEL
@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) bugfix This fixes a bug cleanup labels Jun 3, 2022
@gmao-rreichle gmao-rreichle self-assigned this Jun 3, 2022
@weiyuan-jiang
Copy link
Contributor

I think in MAPL, the data MAPL_UNDEF is ignored and is not counted in averaging calculation

@weiyuan-jiang
Copy link
Contributor

That is, if a given (say, cube-sphere) grid cell includes two tiles, one mineral and one PEATCLSM, then the average of PEATCLSM_WATERLEVEL for the cube-sphere grid cell in question should be that of the PEATCLSM tile, rather than MAPL_UNDEF.

DO you mean the calculation of PEATCLSM_WATERLEVEL should depend on other variables PEATCLSM ?

@gmao-rreichle
Copy link
Contributor Author

I think in MAPL, the data MAPL_UNDEF is ignored and is not counted in averaging calculation

Ok, thanks. We'll assume for now that nothing else needs to be done and verify the assumption in a cube-sphere tile-space experiment.

DO you mean the calculation of PEATCLSM_WATERLEVEL should depend on other variables PEATCLSM ?

No. Water level is computed in tile space in CatchGridComp. The question is simply what we get when we ouput waterlevel in grid space from SurfaceGridComp (where the tile2grid aggregation is done by MAPL Location stream). So if x(tile1) = 0.5 and x(tile2)=NaN, we want x(gridcellA)=0.5, where gridcellA consists of tile1 and tile2.

@bena-nasa
Copy link
Collaborator

bena-nasa commented Jun 6, 2022

@gmao-rreichle If x(tile1) = 0.5 and x(tile2)=NaN, where gridcellA consists of tile1 and tile2. Then the MAPL_LocstreamTransoform will give: x(gridcellA)=0.5

Basically any MAPL_UNDEF in the source tiles does not count towards the destination cell. In that case the weights are renormalized. That is done here:
https://github.com/GEOS-ESM/MAPL/blob/main/base/MAPL_LocStreamMod.F90#L1949

@mbechtold
Copy link

No questions or suggestions. Sounds like a very good idea to me to have PEATCLSM_WATERLEVEL and PEATCLSM_FSWCHANGE output variables. I also appreciate the sign change.

@gmao-rreichle
Copy link
Contributor Author

See below for update on testing.

@sdrabenh: We are ready to start SMAP L4_SM Version 7 reprocessing as soon as we have a new GEOSgcm_GridComp release that includes this bug fix. The PR could be merged immediately, so we could start L4_SM reprocessing without further delay. Or we could wait (probably about a day) for Biljana's test to be completed. In any case, this is a heads-up that it would be very helpful to merge this PR as soon as you can fit it into your schedule.

I'm going to change the status from "draft" to "ready for review" now so we can start collecting approvals from @GEOS-ESM/landice-team and @GEOS-ESM/ocean-team. The changes are strictly limited to export variables for "land" tiles and can be considered trivially 0-diff for all things GCM.

Update on tests completed so far:
From @gmao-qliu, 13 Jun 2022: "The NRv10 and OLv7 tests are both zero diff from before. I also confirm that the WATERABLED and FSWCHANGE output are scientifically identical for peat tiles (with WTD sign reversed)."
From @biljanaorescanin, 13 Jun 2022: "I've run AGCM tests and those are zero diff."
Qing's testing of the changes in the PEATCLSM_* outputs was done on the EASEv2 grid, with 1 tile per grid cell, which is all we need for SMAP L4_SM. We still need to verify that that the tile2grid aggregation works as expected when on the cube-sphere tile space with lat/lon-gridded output. @biljanaorescanin is working on this. Based on the input by @bena-nasa above, we fully expect this to work properly. But any issues there would not affect SMAP L4_SM.

@gmao-rreichle gmao-rreichle removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Jun 13, 2022
@gmao-rreichle gmao-rreichle marked this pull request as ready for review June 13, 2022 20:20
@gmao-rreichle gmao-rreichle requested review from a team as code owners June 13, 2022 20:20
@gmao-rreichle
Copy link
Contributor Author

FINAL update: Based on output created by @biljanaorescanin using "develop" and this branch with NLv5 bcs on the cube-sphere tile space, I verified that PEATCLSM_WATERLEVEL and PEATCLSM_FSWCHANGE are ok. PR is definitely ready to be merged as is.

@sdrabenh sdrabenh merged commit 1046239 into develop Jun 15, 2022
@sdrabenh sdrabenh deleted the bugfix/rreichle/water_table_depth branch June 15, 2022 12:13
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. bugfix This fixes a bug cleanup documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants