-
Notifications
You must be signed in to change notification settings - Fork 313
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
Dynamical lakes and new lake cover map #1073
Conversation
…dynConsBiogeophysMod.f90 cv is kept constant at water cv for simplicity (altough there is a cv for each lake layer) TEMPORARY change
… as an history field. The history field only contains the heat content of the lake water itself, and is a column output.
…namic lake land unit. Use similar to do_transient_crops
…) and accounted for cases where pct urban + pct lake > 100, adjusting pct lake so that total is 100%
…d comments to code!
Thanks, @billsacks for your feedback. I am now in the process of reviewing the diffs of this branch relative to master, and will notify you here, once this is finished and I got some testing done. |
This is a double precision variable on the file, so I'm making it double precision in the code. I think ncdio_pio used to handle data type conversions like this, but now it seems to be corrupting memory: the original code led to errors in decompInitMod, due to what appears to be corruption in some unrelated variable.
I found issue #43, which might interfere with the dynlakes branch (not investigated in detail). Also note, these developments add lake water and heat to the total gridcell water and heat content, and consequently, TWS now includes lake water, as it didn't do before. When starting my simulations @billsacks had an first review and made the following comments: (1) You accidentally added some files. Not a big deal, but something we'll want to clean up before this is merged in eventually: $ git diff --name-status HEAD..Ivanderkelen/dynlakes | grep '^A' | grep -v dynlakeFileMod This was done intentionally, as requested by Erik to be able to update the mksurfdat tool to also take the new lake fraction map I created. (2) It looks like the code you added in surfrdMod – to read the haslake variable – will only work if there is actually a landuse_timeseries file present. Some runs don't have this, which would cause this code to crash. Not important to fix now, but we'll want to fix it eventually. (3) In dynSubgridControl, you prevent running with dynlakes and fates. I think these would be compatible. (We can't currently run with both transient veg and fates, but I think it would work to run with transient lakes & fates.) This originally came from following the harvest example. I deleted the lines preventing it but didn't test it with fates. |
Thanks for bringing that to our attention. My sense is that #43 should be resolved at some point, but it isn't critical for the first iteration of the dynamic lake work. It is likely needed for the correct operation of the methane code with dynamic lakes, though (which is active when running with Bgc compsets, not with Sp [satellite phenology]). If you have time to look into it at some point in the next few months, that would be great, but I don't think that should hold up bringing your initial changes to master. |
This just maintains changes in tools. I am moving the src and bld changes to a different branch so that they can come in separately from the tools changes.
Review of Bill Sacks: Revert non-tools changes, and some minor edits to the tools changes
|
@billsacks instead of having a variable on the file -- why doesn't it just check for the existence of the lake variable on the file? I don't see the need for a separate variable to say that another variable exists. You should be able to check the variable for existence and then use that to set the logical flag for it in the code. |
@ekluzek HASLAKE is a spatial variable that says whether there will be any lake area at any point in the timeseries. |
Ahh, now that makes sense. I should've looked at the code first. But, thanks for that clarification. |
Thank you @ekluzek and @billsacks for looking the review! I am now back and ready to work on this.
|
@Ivanderkelen I should add: please wait to do any major work on mksurfdat.F90: I have a small set of changes coming to master today or early next week (#1119 ) and you'll get conflicts if you try to do some of the rework I suggested before then. So, once #1119 is merged, you should update your branch to the latest version of master, then can do the rework I suggested. |
To maintain consistency in truncating and adjusting landunits between surfdat and landuse.timeseries files. The pct landunits of the surfdat files are saved and reset each year in order to do adjustments from scratch each year.
Add capability for dynamic lakes Adds the capability for dynamic lake areas, read from the landuse_timeseries file. This represents reservoir construction. For now, this capability is off by default. Turning it on requires new fields on the landuse_timeseries file which cannot yet be produced by mksurfdata_map; these new fields will be added in #1073. A substantial part of this tag involved changing the accounting of water and energy in lakes in order to conserve water and energy across landunit transitions while not producing too large adjustment fluxes. This change results in roundoff-level answer changes for all transient cases. The core changes in this tag are from Inne Vanderkelen, in consultation with Bill Sacks. Additional changes are from Bill Sacks, in consultation with Inne Vanderkelen. Resolves #200 Resolves #1140
Add capability for dynamic lakes Adds the capability for dynamic lake areas, read from the landuse_timeseries file. This represents reservoir construction. For now, this capability is off by default. Turning it on requires new fields on the landuse_timeseries file which cannot yet be produced by mksurfdata_map; these new fields will be added in ESCOMP#1073. A substantial part of this tag involved changing the accounting of water and energy in lakes in order to conserve water and energy across landunit transitions while not producing too large adjustment fluxes. This change results in roundoff-level answer changes for all transient cases. The core changes in this tag are from Inne Vanderkelen, in consultation with Bill Sacks. Additional changes are from Bill Sacks, in consultation with Inne Vanderkelen. Conflicts: bld/namelist_files/namelist_defaults_ctsm.xml src/biogeophys/CanopyFluxesMod.F90
|
||
call change_landuse(ldomain, dynpft=.true.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Comparing the changes in this PR with those done by @keerzhang1 in Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579, one difference I see is the removal of this change_landuse call in the time loop in this PR (it was left in place in Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579). It probably doesn't matter hugely, but I'm thinking that it's probably best to leave this call to change_landuse in place, as is done in Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579 .
The relevant changes from this PR are now incorporated into #1579 (by @keerzhang1). I have also made a few notes in that PR summarizing the relevant outstanding discussion in this PR. Therefore, I am closing this PR since it is superseded by #1579 . Thank you, @Ivanderkelen , for all of your work on this PR, which provided a very helpful basis for #1579 ! |
Description of changes
Dynamic lakes representing reservoir construction and new lake cover input map using HydroLAKES and GRanD data.
Update (2020-08-17): this will be split into two separate PRs; this PR will just contain tools changes, and a new PR (which will come to master first) will contain all other changes.
Specific notes
Contributors other than yourself, if any: @billsacks
CTSM Issues Fixed (include github issue #): none
Are answers expected to change (and if so in what way)?
Any User Interface Changes (namelist or namelist defaults changes)?
New namelist handle use_dynlakes is added, off by default
Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).