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

Soil temperatures (TP's) in units of Kelvin throughout #329

Closed
wants to merge 3 commits into from

Conversation

biljanaorescanin
Copy link
Contributor

No description provided.

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin, here are my 2c:

1.) The changes in your branch are probably too extensive. The original objective of #114 was to "convert all soil (and snow?) temperature EXPORT variables outside of catchment.F90 into units of Kelvin." We should probably consider the model diagnostic subroutines (e.g., catch_calc_tp()) to be part of catchment(). For one thing, the diagnostics subroutines are also used in the ./mk_restarts, which would presumably require further edits. Second, the snow temperatures are still a mix of Celsius and Kelvin. Changing snow temperatures to Kelvin throughout would require us to work with the landice group. So it's probably best to stick with the original plan and only change the units for "export" variables. That is, soil temperatures remain in Celsius for all of the scientific computations associated with catchment() and are only converted to Kelvin when the variables become MAPL exports.
Restarting from develop, I created two new branches, one for GEOSldas and one for GEOSgcm_GridComp, with the same name: feature/rreichle_TP_C_to_K.
I successfully tested the GEOSldas branch (except for the CatchCN test, see below, and for Intel only)

2.) My new GEOSgcm_GridComp branch also removes TSURF from the list of variables that are written into offline Catch and CatchCN restart files (per #114). This also seems to work as intended, but (i) it results in an obvious "failure" of the comparison of the restart files and (ii) it may break the code in ./mk_restarts or LDAS_App/mk_GEOSldasRestarts.F90, which is not exercised in the "nightly" tests. Maybe @weiyuan-jiang can comment on what changes might be needed, if any.

3.) I ran my tests only for Catch because the CatchCN test still needs to be updated. Before we can finalize this pull request, we need to fix the CatchCN test and then test this branch. I also didn't do the GNU tests yet, or the AGCM tests.

Please take a look and review my branch (see above). If it makes sense to you, please update the branch associated with this pull request accordingly.

cc: @gmao-jkolassa @rdkoster

@biljanaorescanin
Copy link
Contributor Author

@gmao-rreichle Yes, all make sense and look good. The introduction of the optional tp1_in_Kelvin argument into catch2mwRTM_vars subroutine is a clean way to solve for the differences in the input units from various calls. I'll review the branch and proceed with updates once we fix the CatchCN tests and reiterate with @weiyuan-jiang to see about potential issues listed under 2.)
cc: @gmao-jkolassa @rdkoster

@biljanaorescanin
Copy link
Contributor Author

Addressed with PR #340

@biljanaorescanin biljanaorescanin deleted the feature/borescan_TP_C_to_K_th branch January 19, 2021 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

soil (and snow?) temperatures in units of Kelvin throughout ; do not write TSURF into "offline" catch restarts
2 participants