Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579

keerzhang1 · 2021-12-16T20:57:13Z

Description of changes

This update includes code changes that enable the mksurfdata_map to generate landuse_timeseries with transient urban variables (PCT_URBAN and HASURBAN).

Update (2021-12-21): The changes in #1073 was combined with this PR. Now the code changes enable the mksurfdata_map to generate landuse_timeseries with dynamic urban and lake variables (PCT_URBAN, PCT_LAKE, PCT_URBAN_MAX, and PCT_LAKE_MAX)

Specific notes

For now, the changes in the mksurfdata.pl are hardcoded to simply generate a landuse_timeseries*. txt file containing urban raw data locations that I specified.

Contributors other than yourself, if any: @olyson @fang-bowen @Face2sea

CTSM Issues Fixed (include github issue #): #1445

Are answers expected to change (and if so in what way)?
The surface datasets will be different.
Any User Interface Changes (namelist or namelist defaults changes)?
No.
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).

In normalizencheck_landuse, still reduce crop and natveg proportionally when urban exist.

changed a txt file name

Add PCT_NATVEG for testing purpose

billsacks

This basically looks great! Thanks a lot for your work on it @keerzhang1 @olyson @fang-bowen and @Face2sea !

I have a few comments below. I'm curious what you think about my suggestion to write PCT_URB_MAX instead of HASURBAN. The comment on the perl code is probably something we'll do on our end.

The biggest thing I see at this point is figuring out a plan to merge this with #1564 and #1073 . (Now I'm sorry that I suggested two different PRs for this and #1564 because I see this isn't as clean as I was hoping.) I'll follow up on this by email.

billsacks · 2021-12-20T19:39:11Z

tools/mksurfdata_map/src/mkfileMod.F90

+       call ncd_def_spatial_var(ncid=ncid, varname='HASURBAN', xtype=nf_int, &
+            lev1name='numurbl', &
+            long_name='whether the grid ever has urban during the considered time period', units='unitless')


I'm starting to think it would be better to use PCT_URBAN_MAX instead of HASURBAN. See Improve mechanism for determining where to allocate memory for urban when running with dynamic urban #1572 (comment) for details.

Done. I've deleted HASURBAN and used PCT_URBAN_MAX instead.

billsacks · 2021-12-20T21:27:18Z

tools/mksurfdata_map/src/mksurfdat.F90

+    ! Save special land unit areas of surface dataset 
+    pctlak_orig(:) = pctlak(:)
+    pctwet_orig(:) = pctwet(:)
+    pcturb_orig(:) = pcturb(:)


It looks like pcturb_orig can be removed, since it isn't referenced anywhere. (And once this is merged with Dynamical lakes and new lake cover map #1073 pctlak_orig can also be removed.)

Thanks! Done.

billsacks · 2021-12-20T22:11:16Z

tools/mksurfdata_map/mksurfdata.pl

+        my $urbanyr = "/glade/scratch/keerzhang/archive/BNU_NoAdjust/05deg_".$yr."_new_NoAdjust.nc";
+        chomp( $urbanyr);
+        printf $fh_landuse_timeseries $dynpft_format, $urbanyr, $yr; # I hard coded this part just to generate a txt file with urban raw data file locations
+        if ( $yr % 100 == 0 ) {                                      # And note that I made no change to the "landuse_timeseries_override_$desc.txt" because I am not sure how to deal with the the 'pft_override' option


[deferring] We'll need a final solution here (@ekluzek )

billsacks · 2021-12-20T22:14:21Z

tools/mksurfdata_map/src/mksurfdat.F90

-
+       do n = 1, ns_o
+          do k =1, numurbl
+             if (pcturb_max(n,k) > 1.e-3_r8) then 


I think it could be problematic to have a hard-coded threshold of 1e-3 here: I feel like this should just check for pcturb_max > 0. (There would be a problem if hasurban is set to 0 but pcturb is large enough to trigger the attempted creation of an urban landunit. Maybe the solution is to do a truncation to 0 of too-small urban (e.g., less than 1e-3) at some point prior to this, and then this check here can just check for > 0?) However, this may be irrelevant if you go with my suggestion elsewhere of getting rid of HASURBAN and just writing PCT_URBAN_MAX to the landuse timeseries file – in which case this whole block of code would be removed.

Yes, this block of code is deleted since we now use PCT_URBAN_MAX.

billsacks

This looks great now. Thanks a lot for the recent changes.

I think the next step will be combining the changes from #1073 with this PR. My feeling is that the best way to do that would be to bring the #1073 changes into this branch (so incorporating them into this PR), but I'm open to other workflows if you feel something else would be easier.

keerzhang1 · 2021-12-22T16:24:53Z

This looks great now. Thanks a lot for the recent changes.

I think the next step will be combining the changes from #1073 with this PR. My feeling is that the best way to do that would be to bring the #1073 changes into this branch (so incorporating them into this PR), but I'm open to other workflows if you feel something else would be easier.

OK, Thanks! I incorporated the changes from #1073 into this PR #1579. At this point, the PCT_URBAN_MAX and PCT_LAKE_MAX were saved in the landuse.timeseries file. Please let me know if anything needs to be changed.

billsacks

This looks great! Thanks a lot for doing it, @keerzhang1 - and for doing it so quickly!

I just noticed one minor copy-and-paste error - see my inline comment.

I'm going to add a few more comments and todo notes to this PR about next steps, but they are mainly notes for ourselves (@olyson @ekluzek and me). Thanks again for your great work on this!

billsacks · 2021-12-22T17:20:42Z

tools/mksurfdata_map/src/mkfileMod.F90


+       call ncd_def_spatial_var(ncid=ncid, varname='PCT_LAKE_MAX', xtype=xtype, &
+            long_name='maximum percent lake for each density type', units='unitless')


Minor copy and paste error:

Suggested change

long_name='maximum percent lake for each density type', units='unitless')

long_name='maximum percent lake', units='unitless')

OK, I've corrected it.

billsacks · 2021-12-22T17:55:42Z

One thing we'll need to do before finalizing this is to get the mksurfdata.pl changes into a final form (@ekluzek ). See also some related comments in Dynamical lakes and new lake cover map #1073 about new sample namelist files (e.g., Dynamical lakes and new lake cover map #1073 (comment)).
- Also note some discussion in Dynamical lakes and new lake cover map #1073 about how to handle different sets of years between the different datasets. I believe the conclusion (based on recent discussions with @ekluzek ) is that we'll have some logic in mksurfdata.pl to replicate years as necessary for datasets without the full set of years available. i.e., the text file produced by mksurfdata.pl will have a line for each dataset (pfts, harvest, urban and lake) for each year, but some of those lines may be identical for datasets without the full complement of years available. (e.g., see Dynamical lakes and new lake cover map #1073 (comment))

billsacks · 2021-12-22T18:09:48Z

@keerzhang1 @olyson @ekluzek - as we (Erik, Keith, Negin and I) discussed, I have changed the base branch for this PR to the new ctsm5.2.mksurfdata branch, which will integrate a number of changes needed for mksurfdata_map for CTSM5.2. I noticed that there are some changes to the mksurfdata_map code on that ctsm5.2.mksurfdata branch (from changes made to CTSM master over the last few months) beyond what was used as the base for this PR. I don't think there are any conflicts yet, but @olyson I think it would be a good idea to merge the latest ctsm5.2.mksurfdata into this before making any further changes.

olyson · 2021-12-22T18:48:50Z

Hmm, ok @billsacks .
So would these be the appropriate steps to do that?

git clone https://github.com/ESCOMP/CTSM.git ./ctsm_dynurb_mksurf
cd ctsm_dynurb_mksurf/
git remote add keerzhang1 https://github.com/keerzhang1/ctsm.git
git fetch keerzhang1
git checkout dynurb_mksurf
git fetch --tags origin
git merge origin/ctsm5.2.mksurfdata
./manage_externals/checkout_externals
git push keerzhang1 dynurb_mksurf

The "origin" in front of ctsm5.2.mksurfdata seems to be necessary but I don't understand why.

billsacks · 2021-12-22T22:55:49Z

So would these be the appropriate steps to do that?

git clone https://github.com/ESCOMP/CTSM.git ./ctsm_dynurb_mksurf
cd ctsm_dynurb_mksurf/
git remote add keerzhang1 https://github.com/keerzhang1/ctsm.git
git fetch keerzhang1
git checkout dynurb_mksurf
git fetch --tags origin
git merge origin/ctsm5.2.mksurfdata
./manage_externals/checkout_externals
git push keerzhang1 dynurb_mksurf

The "origin" in front of ctsm5.2.mksurfdata seems to be necessary but I don't understand why.

Sorry for the delayed reply. Yes, that looks right. You need the "origin" in front of "ctsm5.2.mksurfdata" because it's a branch on the ESCOMP remote. Maybe you're thinking that it's a tag, and/or are used to merging tags in. If you want to merge a remote branch in, and don't have a local copy of the branch already checked out, then you need to give the remote name like this.

olyson · 2021-12-22T23:15:36Z

Thanks for the explanation. The merge seems to have gone well, no conflicts, however, I don't seem to have push permission for this branch. @keerzhang1 or @billsacks , can you give me permission?

billsacks · 2021-12-22T23:16:13Z

@olyson @ekluzek this is in good shape now in terms of the merge with #1073. I can see three possible paths forward at this point, and I'm wondering if you have feelings on which would be best. Maybe we resolved this in our discussions yesterday, but if we did, I can't remember what we decided:

Keith applies his changes on top of this, in this PR. This would be the simplest approach, but would make the later separation of feature additions from answer changes more difficult than in either of the following approaches. This is okay with me if it's okay with @ekluzek .
Run tools testing on this and merge it to the ctsm5.2 mksurfdata branch, if we expect tools testing to pass at this point. Then Keith could start his branch off of the ctsm5.2 mksurfdata branch and open a PR directly into that. However, I'm not sure if we do expect tools testing to pass on this PR yet, because of the temporary changes in the perl, and the possible need to update the default namelist. Also, it might make things easier long-term if we wait to merge this until the perl code is in the final form so that that all stays self-contained in this PR.
Keith makes a branch off of this and opens a PR into this branch (i.e., opens a PR into the dynurb_mksurf branch), which we can later update to be into the ctsm5.2 mksurfdata branch once the dynurb_mksurf branch is eventually merged. i.e., we'll have this PR 1579 and then Keith will open a separate PR into this PR's branch. This is probably the cleanest solution and will offer the most flexibility moving forward, but will require managing two separate PRs / branches until they're merged. Also, if we go with this approach, I think it will be best if the branch for this PR (1579) is moved to ESCOMP so that Keith's PR can live in ESCOMP rather than on @keerzhang1 's fork. That, in turn, would (I think) require closing this PR and opening a new one (and moving over all still-relevant comments); that's not a big deal, though, and I'm willing to take the lead on that.

My gut feeling is that (1) or (3) is best, with (3) feeling more "right" but (1) being simpler. @ekluzek and @olyson do you have preferences?

billsacks · 2021-12-22T23:17:17Z

I don't seem to have push permission for this branch. @keerzhang1 or @billsacks , can you give me permission?

I don't think I can do this; @keerzhang1 will have to. Or this would be resolved if we go with option (3) in my last comment, which would involve moving this branch to ESCOMP for now.

billsacks

I'm marking this as approved, even though we will still need to clean up the perl. (I'm assuming that @ekluzek will do that at some point because he's most familiar with that code, and so my review work on this PR is done now, which is why I'm marking it as approved from my point of view.)

olyson · 2021-12-22T23:41:45Z

I think option 2 is out because right now the urban landuse data only starts in year 2000, so if the tools testing tests the generation of transient datasets (1850-2014), I think it will fail based on that alone.
Option 1 definitely sounds simpler but as you say doesn't allow for easy separation of feature additions from answer changes. Although I'm not sure how we prove that this PR doesn't change answers since we can't really run the tools testing on it? Does the tools testing actually check for answer changes?
Option 3 is indeed more complicated so I might need some specific instructions on how to implement that.

billsacks · 2021-12-23T00:09:10Z

I think option 2 is out because right now the urban landuse data only starts in year 2000, so if the tools testing tests the generation of transient datasets (1850-2014), I think it will fail based on that alone.

Thanks, good point.

Option 1 definitely sounds simpler but as you say doesn't allow for easy separation of feature additions from answer changes. Although I'm not sure how we prove that this PR doesn't change answers since we can't really run the tools testing on it?

I should have clarified that, if we go with option (3), then we would wait to merge this current PR (or its new equivalent using a branch on ESCOMP) until everything is in place for the tools testing to pass. So (3) would involve:

Move the dynurb_mksurf branch to ESCOMP
Open a new PR from that ESCOMP/dynurb_mksurf into ctsm5.2.mksurfdata, moving over the relevant comments from this PR and then closing this PR
Keith starts a branch from ESCOMP/dynurb_mksurf, and opens a PR into ESCOMP/dynurb_mksurf
(Eventually) Erik makes any necessary changes to the perl code and the xml database to allow generation of transient datasets with transient urban & lake
We run tools testing on the updated version of 1579, which should now pass
The updated version of 1579 is merged to ctsm5.2.mksurfdata
Keith's PR is updated to the latest version of ctsm5.2.mksurfdata, and has the tools testing run on it
Keith's PR is merged to ctsm5.2.mksurfdata

Does the tools testing actually check for answer changes?

Yes, it checks for answer changes in the generated surface datasets

Option 3 is indeed more complicated so I might need some specific instructions on how to implement that.

I'd be happy to help with the git / GitHub details if we go with this option.

keerzhang1 · 2021-12-23T00:17:59Z

Thanks for the explanation. The merge seems to have gone well, no conflicts, however, I don't seem to have push permission for this branch. @keerzhang1 or @billsacks , can you give me permission?

I just added @olyson as a collaborator. Do you have the push permission now?

olyson · 2021-12-23T00:19:22Z

Thanks for the invitation @keerzhang1, I now have push access. I'll hold off until we decide which option to go with.

…mksurf

billsacks · 2021-12-29T21:52:48Z

@olyson @keerzhang1 @ekluzek - I'm going to do a slight modification of the plan for (3) that I laid out above, to better communicate the status of this PR: As far as I'm concerned @keerzhang1 's work on this PR is complete (thank you!). I want to give Keer the satisfaction of having a merged PR while also indicating that this branch is moving on to phase 2, which just involves some work on our end (primarily in the perl code), and also moving the branch into ESCOMP (both to facilitate @ekluzek continuing to work on it, and to allow @olyson to open a PR into it). To accomplish this, I have started a new branch in the ESCOMP repository named dynurb_mksurf_part2 which is for now identical to the ctsm5.2.mksurfdata branch. I have changed the base branch for this PR to be dynurb_mksurf_part2. Now I will merge this PR.

The end result will be the same as if I moved the dynurb_mksurf branch into ESCOMP (renaming it to add "_part2"), but this approach will show this PR as merged rather than just closed, which I think better indicates the status given that @keerzhang1 has seen this through to completion of everything we have asked. Thank you again @keerzhang1 !

billsacks · 2021-12-29T23:56:44Z

This PR has been superseded by #1586 . I am moving the still-relevant comments from here into #1586 .

keerzhang1 added 13 commits November 16, 2021 20:58

add PCT_URBAN in landuse.timeseries data

fa89274

In normalizencheck_landuse, still reduce crop and natveg proportionally when urban exist.

changed a txt file name

5c69e8c

changed a txt file name

Add PCT_NATVEG for testing purpose

71318fa

Add PCT_NATVEG for testing purpose

Delete variable PCT_NATVEG in landuse.timeseries file

88f822a

Add hasurban

87b5fbd

Fix type (double--int)

f506bc9

fix syntax error

64abee1

Delete PCT_URBAN_MAX

b3a6e5a

fixed a wrong indentation

0dc8ca4

Declare hasurban as a double variable

f57a3e4

Change HASURBAN type as integer

da08140

Add some comments to emphasize hard coding

5316eb5

Change the pcturb_max threshold for HASURBAN

b3d654a

olyson added this to the ctsm5.2.0 milestone Dec 16, 2021

olyson added the tag: enh - new science label Dec 16, 2021

billsacks requested changes Dec 20, 2021

View reviewed changes

keerzhang1 added 2 commits December 21, 2021 09:51

Delete HASURBAN

6f87e1c

Delete pcturb_orig

b51b3b9

billsacks approved these changes Dec 21, 2021

View reviewed changes

keerzhang1 changed the title ~~Dynurb mksurf~~ Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban Dec 22, 2021

Merge ESCOMP#1073 with this PR ESCOMP#1579(Add dynamic lake variables)

09b35c5

keerzhang1 changed the title ~~Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban~~ Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake Dec 22, 2021

billsacks requested changes Dec 22, 2021

View reviewed changes

billsacks changed the base branch from master to ctsm5.2.mksurfdata December 22, 2021 18:07

billsacks mentioned this pull request Dec 22, 2021

Change HASLAKE to PCT_LAKE_MAX #1582

Closed

Correct copy and paste error

fc7119d

billsacks approved these changes Dec 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/ctsm5.2.mksurfdata' into dynurb_…

7ba893e

…mksurf

billsacks changed the base branch from ctsm5.2.mksurfdata to dynurb_mksurf_part2 December 29, 2021 21:42

billsacks merged commit 7ba893e into ESCOMP:dynurb_mksurf_part2 Dec 29, 2021

billsacks mentioned this pull request Dec 29, 2021

Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1586

Merged

samsrabin added enhancement new capability or improved behavior of existing capability science Enhancement to or bug impacting science labels Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579

Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579

keerzhang1 commented Dec 16, 2021 •

edited

Loading

billsacks left a comment

billsacks Dec 20, 2021 •

edited

Loading

keerzhang1 Dec 21, 2021

billsacks Dec 20, 2021 •

edited

Loading

keerzhang1 Dec 21, 2021

billsacks Dec 20, 2021 •

edited

Loading

billsacks Dec 20, 2021 •

edited

Loading

keerzhang1 Dec 21, 2021

billsacks left a comment

keerzhang1 commented Dec 22, 2021

billsacks left a comment

billsacks Dec 22, 2021 •

edited

Loading

keerzhang1 Dec 22, 2021

billsacks commented Dec 22, 2021 •

edited

Loading

billsacks commented Dec 22, 2021

olyson commented Dec 22, 2021

billsacks commented Dec 22, 2021

olyson commented Dec 22, 2021

billsacks commented Dec 22, 2021

billsacks commented Dec 22, 2021

billsacks left a comment

olyson commented Dec 22, 2021

billsacks commented Dec 23, 2021

keerzhang1 commented Dec 23, 2021

olyson commented Dec 23, 2021

billsacks commented Dec 29, 2021

billsacks commented Dec 29, 2021


		call ncd_def_spatial_var(ncid=ncid, varname='PCT_LAKE_MAX', xtype=xtype, &
		long_name='maximum percent lake for each density type', units='unitless')

	long_name='maximum percent lake for each density type', units='unitless')
	long_name='maximum percent lake', units='unitless')

Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579

Enable the mksurfdata_map to generate landuse_timeseries for dynamic urban and lake #1579

Conversation

keerzhang1 commented Dec 16, 2021 • edited Loading

Description of changes

Specific notes

billsacks left a comment

Choose a reason for hiding this comment

billsacks Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

keerzhang1 Dec 21, 2021

Choose a reason for hiding this comment

billsacks Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

keerzhang1 Dec 21, 2021

Choose a reason for hiding this comment

billsacks Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

billsacks Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

keerzhang1 Dec 21, 2021

Choose a reason for hiding this comment

billsacks left a comment

Choose a reason for hiding this comment

keerzhang1 commented Dec 22, 2021

billsacks left a comment

Choose a reason for hiding this comment

billsacks Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

keerzhang1 Dec 22, 2021

Choose a reason for hiding this comment

billsacks commented Dec 22, 2021 • edited Loading

billsacks commented Dec 22, 2021

olyson commented Dec 22, 2021

billsacks commented Dec 22, 2021

olyson commented Dec 22, 2021

billsacks commented Dec 22, 2021

billsacks commented Dec 22, 2021

billsacks left a comment

Choose a reason for hiding this comment

olyson commented Dec 22, 2021

billsacks commented Dec 23, 2021

keerzhang1 commented Dec 23, 2021

olyson commented Dec 23, 2021

billsacks commented Dec 29, 2021

billsacks commented Dec 29, 2021

keerzhang1 commented Dec 16, 2021 •

edited

Loading

billsacks Dec 20, 2021 •

edited

Loading

billsacks Dec 20, 2021 •

edited

Loading

billsacks Dec 20, 2021 •

edited

Loading

billsacks Dec 20, 2021 •

edited

Loading

billsacks Dec 22, 2021 •

edited

Loading

billsacks commented Dec 22, 2021 •

edited

Loading