Extend CNFireMethodMod with a FATESFireData class #991

slevis-lmwg · 2020-04-24T01:51:54Z

Description of changes

Allow FATES access to CTSM's infrastructure that reads lightning and
human population data sets for use in SPITFIRE.

Following @ekluzek's detailed recommendations in #982 #983 .

Specific notes

Contributors other than yourself, if any:
@ekluzek @rgknox @ckoven @lmkueppers @jkshuman

CTSM Issues Fixed (include github issue #):
#982 #983
Also relates directly to issue NGEET/fates#562
and to pull request NGEET/fates#635

Are answers expected to change (and if so in what way)?
Not in non-FATES simulations.
Not in FATES simulations using use_fates_spitfire < 2
Yes in FATES simulations using use_fates_spitfire >= 2

Any User Interface Changes (namelist or namelist defaults changes)?
Changing use_fates_spitfire from logical to integer with valid values of:
0 for no fire
1 for spitfire with default lightning constant
2 for spitfire with Li et al lightning data
3 for spitfire with successful ignitions data
as recommended by @rgknox here

Testing performed, if any:
Code builds. No further testing, yet.

@ekluzek

Following @ekluzek's recommendations in ESCOMP#982. Purpose: Allow FATES access to CTSM's infrastructure that reads lightning and human population data sets for use in SPITFIRE.

src/utils/clmfates_interfaceMod.F90

ekluzek

@slevisconsulting there's a little bit of misunderstanding about the object oriented class structure here. I gave some guidance, but we should probably go through this again after you've gone through it. But, it does look close!

bld/namelist_files/namelist_defaults_clm4_5.xml

bld/namelist_files/namelist_definition_clm4_5.xml

src/biogeochem/FATESFireNoDataMod.F90

src/main/clm_initializeMod.F90

ekluzek · 2020-04-26T04:01:15Z

src/main/clm_initializeMod.F90

@@ -606,6 +614,14 @@ subroutine initialize2( )
       call crop_inst%initAccVars(bounds_proc)
    end if

+    ! use_fates_spitfire is assigned an integer value in the namelist
+    ! see bld/namelist_files/namelist_definition_clm4_5.xml for details
+    if (use_fates_spitfire > 1) then


Just call the generic one rather than the specific one

Same comment.

src/main/clm_instMod.F90

src/utils/clmfates_interfaceMod.F90

slevis-lmwg · 2020-04-27T00:34:03Z

@ekluzek pls see my responses and revisions. If I am on the wrong track, let's definitely have a meeting.

ekluzek

I updated my comments and responded to your questions...

bld/namelist_files/namelist_defaults_clm4_5.xml

src/biogeochem/FATESFireNoDataMod.F90

src/main/clm_driver.F90

src/main/clm_initializeMod.F90

src/main/clm_instMod.F90

src/main/clm_initializeMod.F90

ekluzek · 2020-05-04T20:40:07Z

@slevisconsulting so Init2 and InterpFileInputs should go into clmfates_interface, rather than to the extensions of the Fire classes for FATES.

I think it will help to have the general conversation about the Fire class methods and the refactoring I'm proposing to make sure you are clear on some of the OO concepts being used here. So I think we should plan on that before you proceed forward. We can make sure we talk about what I specifically plan on doing, as well as what you need to do here.

I think most of the conversations can be resolved now. So I'll go through and make sure I do that. There are only a couple that will be outstanding. Once we go through there will only be a couple last changes to make.

Removed from FATESFire[No]DataMod. Added to clmfates_interfaceMod. Updated calls in clm_driver and clm_initialize.

...since FATES only needs lightning for now.

src/utils/clmfates_interfaceMod.F90

src/main/clm_driver.F90

ekluzek · 2020-05-08T19:40:48Z

I am seeing changes that are needed now that I'm looking directly at the code. There's a couple ways to proceed. I can figure out the changes and post the diff needed, or I could try to explain inline in github, or. I could submit a PR to your branch, or lastly if you authorize me -- I can push directly to your branch. Which of those sound like the best solution?

src/main/clm_initializeMod.F90

slevis-lmwg · 2020-05-08T19:53:09Z

I am seeing changes that are needed now that I'm looking directly at the code. There's a couple ways to proceed. I can figure out the changes and post the diff needed, or I could try to explain inline in github, or. I could submit a PR to your branch, or lastly if you authorize me -- I can push directly to your branch. Which of those sound like the best solution?

I think simplest for me would be to authorize you to push directly to my branch. How do I do that?

src/main/clm_instMod.F90

ekluzek · 2020-05-08T19:59:52Z

Go to your fork to CTSM on github. Go to settings tab on the right top, and then "manage access" option on the left down one. Then you can add me as a collaborator. At some point it asks you to enter your password.

It'll send me an email and then I just need to accept it. I've done this with several people and it makes it a little easier to work on the same code together. Sometimes if there's something more extensive I'll still do a Pull Request, to their branch, so they can code review it beforehand. But, here we are talking about small changes. So I'll get it working for a simple case and then push my commits to your branch. I'll try to explain as well as I can in the commit notes.

Thanks.

slevis-lmwg · 2020-05-08T20:23:39Z

It'll send me an email and then I just need to accept it.

Ok, you should get a email.

Thanks @ekluzek

…terface These are the changes that I see that are needed to keep fates_fire_data_method local to clmfates_interface. Whenever fates_fire_data is accessed it has to be done through clmfates_interface, because that's the object that contains it and has it instantiated. We also will want it to be private to clmfates_interface, so it can't be accessed or changed outside of there. There were some stubs here to pass specific instances of the class types before, but they weren't completed. So those are all removed and the generic method is being accessed so that you don't have to know which specific instance is being used. Testing: I haven't tested it yet. But, will before pushing

Testing with ./create_test ERS_D_Lm36.1x1_brazil.I2000Clm50FatesCruGs.cheyenne_intel.clm-Fates_nat_and_anthro_ignitions

slevis-lmwg · 2020-07-12T20:02:15Z

bld/CLMBuildNamelist.pm

@@ -945,18 +945,12 @@ sub setup_cmdl_fire_light_res {
    }
    if ( defined($fire_method) && $fire_method eq "nofire" ) {
       $nl_flags->{$var} = ".false.";
-    } elsif ( &value_is_true($nl->get_value('use_cn')) ) {
+#   } elsif ( &value_is_true($nl->get_value('use_cn')) || $nl_flags->{'fates_spitfire_mode'} > 1 ) {
+    } elsif ( &value_is_true($nl->get_value('use_cn')) || &value_is_true($nl->get_value('use_fates')) ) {


New line 949 works

} elsif ( &value_is_true($nl->get_value('use_cn')) || &value_is_true($nl->get_value('use_fates')) ) {

However, I wish the commented-out line above it would work because it would narrow things down, but it doesn't work. The run proceeds to the next else and sets cnfireson = .false..

@ekluzek do you see an obvious problem with new line 948?

slevis-lmwg · 2020-07-12T23:49:38Z

FATES testing passes except for the restart test that was already failing as reported in #667 .

This PR is now ready for review again:
@glemieux do you recommend the reviews be done before the upcoming re-basing?
Either way, I am requesting reviews from @ekluzek and @glemieux . All others are welcome to review if interested.

glemieux · 2020-07-13T04:55:50Z

@slevisconsulting yes, ideally the reviews would be completed before the rebase so as to bring in the a consistent set of code that includes review change requests. That said, if there are non-critical change requests that could be brought in later without much hassle and tests are all passing, I'm inclined to rebase this sooner rather that later. Can you point me to the test directory on cheyenne for confirmation, please?

slevis-lmwg · 2020-07-14T01:48:14Z

Can you point me to the test directory on cheyenne for confirmation, please?

You prob. already know this: While Cheyenne is down, you can access the same directory from Casper:
/glade/scratch/slevis/clmfates_tests

Note that this test
ERS_D_Lm12.1x1_brazil.I2000Clm50FatesCruGs.cheyenne_intel.clm-Fates_nat_and_anthro_ignitions
came back as a FAIL because it timed out. I ran a longer variation on this test that PASSed:
/glade/scratch/slevis/ERS_D_Lm36.1x1_brazil.I2000Clm50FatesCruGs.cheyenne_intel.clm-Fates_nat_and_anthro_ignitions.20200712_122433_bhgfz5

glemieux · 2020-07-14T15:29:00Z

Can you point me to the test directory on cheyenne for confirmation, please?

You prob. already know this: While Cheyenne is down, you can access the same directory from Casper:

I didn't know that; thanks for the tip!

jkshuman

@slevisconsulting I added a few comments and questions.

bld/namelist_files/namelist_definition_clm4_5.xml

jkshuman · 2020-07-14T17:34:57Z

bld/unit_testers/build-namelist_test.pl

@@ -822,7 +822,7 @@ sub make_env_run {
                                     conopts=>"-phys clm4_0",
                                   },
     "usespitfireButNOTFATES"    =>{ options=>"-envxml_dir . -no-megan",


does this allow use spitfire without FATES? The name suggests that to me...

@jkshuman this belongs to a list of options that trigger namelist FAILs. The specific one triggers a failure if fates_spitfire_mode > 0 and -bgc has not been set to fates.

cime_config/testdefs/testmods_dirs/clm/Fates_nat_and_anthro_ignitions/README

jkshuman · 2020-07-14T17:57:53Z

src/main/clm_initializeMod.F90

@@ -474,6 +474,12 @@ subroutine initialize2( )
       end if
    else
       call SatellitePhenologyInit(bounds_proc)
+
+       ! fates_spitfire_mode is assigned an integer value in the namelist
+       ! see bld/namelist_files/namelist_definition_clm4_5.xml for details


Suggested change

! see bld/namelist_files/namelist_definition_clm4_5.xml for details

! see bld/namelist_files/namelist_definition_clm4_5.xml for details

! ignitions: 1=constant, 2=lightning, 3=obs, 4=lightning & anthro

I'm leaning against this one because it introduces the risk of being missed in a future update of fates_spitfire_mode.

Though I see the benefit of including some explanation of the > 1 and maybe the naming of these various modes (that I introduced elsewhere) needs to be used here, as well.

valid point, though it may be worth a quick explanation in more than 1 central location

I replaced the > 1 with > scalar_lightning in the latest commit.

jkshuman · 2020-07-14T17:59:00Z

src/main/clm_varctl.F90

@@ -207,7 +207,7 @@ module clm_varctl
  integer, public            :: fates_parteh_mode = -9                 ! 1 => carbon only
                                                                       ! 2 => C+N+P (not enabled yet)
                                                                       ! no others enabled
-  logical, public            :: use_fates_spitfire = .false.           ! true => use spitfire model
+  integer, public            :: fates_spitfire_mode = 0                ! > 0 => use spitfire model: see bld/namelist_files/namelist_definition_clm4_5.xml for details


Suggested change

integer, public :: fates_spitfire_mode = 0 ! > 0 => use spitfire model: see bld/namelist_files/namelist_definition_clm4_5.xml for details

integer, public :: fates_spitfire_mode = 0 ! > 0 => use spitfire model: ignitions: 1=constant, 2=lightning, 3=obs, 4=lightning & anthro

Same as my most recent comment:

Name the various modes in a central location and then use them in all cases like this instead of hardwiring.

I completed this in the current PR's latest commit and in NGEET/fates#635.

The model builds, but I get a runtime system error in my cesm.log that looks like a problem leftover from cheyenne's downtime. What do you suggest @ekluzek :

dplace failed. Verify that the numatools module is loaded.
MPT ERROR: could not run executable (case #1)
(HPE MPT 2.19 02/23/19 05:31:12)

Good news:
Whatever this was, it's been fixed, and my test now runs :-)

slevis-lmwg · 2020-07-14T20:39:46Z

Can you point me to the test directory on cheyenne for confirmation, please?

You prob. already know this: While Cheyenne is down, you can access the same directory from Casper:

I didn't know that; thanks for the tip!

@glemieux I should have pointed out that today specifically Casper will also be down. Sorry for not mentioning sooner...

@jkshuman

Clearer documentation suggested by @jkshuman. Co-authored-by: jkshuman <[email protected]>

@jkshuman

…nitions/README Clearer documentation suggestion by @jkshuman. Co-authored-by: jkshuman <[email protected]>

src/main/clm_varctl.F90

Clearer comment. Co-authored-by: jkshuman <[email protected]>

src/main/clm_varctl.F90

Comment clarification. Co-authored-by: jkshuman <[email protected]>

slevis-lmwg · 2020-07-17T22:03:20Z

@slevisconsulting yes, ideally the reviews would be completed before the rebase so as to bring in the a consistent set of code that includes review change requests. That said, if there are non-critical change requests that could be brought in later without much hassle and tests are all passing, I'm inclined to rebase this sooner rather that later. Can you point me to the test directory on cheyenne for confirmation, please?

@glemieux you may have seen that I made some minor revisions in response to @jkshuman 's review. I have a few more to make once cheyenne comes up. I just want to make sure that you will let me know when I should stop further modifications to avoid conflicts with your re-basing.

glemieux · 2020-07-17T22:19:32Z

Thanks for the heads up @slevisconsulting

slevis-lmwg · 2020-07-20T18:08:26Z

@glemieux I reran FATES testing successfully after my revisions in response to @jkshuman 's reviews.

Testing directory:
/glade/scratch/slevis/clmfates_tests

Updating externals_clm configuration to new tag

Extend CNFireMethodMod with a FATESFireData class

51f3107

Following @ekluzek's recommendations in ESCOMP#982. Purpose: Allow FATES access to CTSM's infrastructure that reads lightning and human population data sets for use in SPITFIRE.

slevis-lmwg commented Apr 26, 2020

View reviewed changes

src/utils/clmfates_interfaceMod.F90 Outdated Show resolved Hide resolved

ekluzek requested changes Apr 26, 2020

View reviewed changes

Revisions based on code review by @ekluzek

af1f812

ekluzek requested changes Apr 28, 2020

View reviewed changes

ekluzek reviewed Apr 28, 2020

View reviewed changes

src/main/clm_instMod.F90 Outdated Show resolved Hide resolved

ekluzek reviewed Apr 28, 2020

View reviewed changes

src/main/clm_initializeMod.F90 Outdated Show resolved Hide resolved

Revisions part 2 based on comments by @ekluzek @jkshuman @lmkueppers

4e7a95a

slevis-lmwg mentioned this pull request Apr 29, 2020

Use lightning data coming in from the CTSM NGEET/fates#635

Merged

5 tasks

Revisions part 3 based on @ekluzek's review

fb2119b

ekluzek assigned slevis-lmwg and ekluzek May 4, 2020

ekluzek added PR status: work in progress enhancement new capability or improved behavior of existing capability labels May 4, 2020

slevis-lmwg added 3 commits May 7, 2020 16:13

Move InterpFileInputs and Init2 to clmfates_interfaceMod

78fb2db

Removed from FATESFire[No]DataMod. Added to clmfates_interfaceMod. Updated calls in clm_driver and clm_initialize.

Added if (use_cn) around reading/interpolating population density

67b82a1

...since FATES only needs lightning for now.

Corrections that reduce the number of compilation errors to two

2fc0ede

slevis-lmwg commented May 8, 2020

View reviewed changes

src/utils/clmfates_interfaceMod.F90 Outdated Show resolved Hide resolved

Corrections for the code to compile

49d2ee6

slevis-lmwg commented May 8, 2020

View reviewed changes

src/main/clm_driver.F90 Outdated Show resolved Hide resolved

ekluzek reviewed May 8, 2020

View reviewed changes

src/main/clm_initializeMod.F90 Outdated Show resolved Hide resolved

ekluzek reviewed May 8, 2020

View reviewed changes

src/main/clm_instMod.F90 Outdated Show resolved Hide resolved

ekluzek added 2 commits May 8, 2020 14:49

Update to Sam's FATES branch for the lightning reading

261a01c

Small corrections to get test simulation to run

e125730

Testing with ./create_test ERS_D_Lm36.1x1_brazil.I2000Clm50FatesCruGs.cheyenne_intel.clm-Fates_nat_and_anthro_ignitions

slevis-lmwg commented Jul 12, 2020

View reviewed changes

slevis-lmwg added 2 commits July 12, 2020 14:20

Added some comments to the code

b583b4f

Changed walltime on new test to avoid timeout

3a08127

slevis-lmwg requested a review from rgknox July 12, 2020 23:59

jkshuman reviewed Jul 14, 2020

View reviewed changes

slevis-lmwg and others added 2 commits July 14, 2020 14:40

Update bld/namelist_files/namelist_definition_clm4_5.xml

4cd75f6

Clearer documentation suggested by @jkshuman. Co-authored-by: jkshuman <[email protected]>

Update cime_config/testdefs/testmods_dirs/clm/Fates_nat_and_anthro_ig…

d1b7f16

…nitions/README Clearer documentation suggestion by @jkshuman. Co-authored-by: jkshuman <[email protected]>

jkshuman reviewed Jul 14, 2020

View reviewed changes

src/main/clm_varctl.F90 Outdated Show resolved Hide resolved

Update src/main/clm_varctl.F90

f68ac40

Clearer comment. Co-authored-by: jkshuman <[email protected]>

jkshuman reviewed Jul 14, 2020

View reviewed changes

src/main/clm_varctl.F90 Outdated Show resolved Hide resolved

Update src/main/clm_varctl.F90

89cdc67

Comment clarification. Co-authored-by: jkshuman <[email protected]>

Made spitfire_mode flags public for use throughout CTSM-FATES

8f3d430

updating externals_clm configuration

4596717

glemieux removed the PR status: work in progress label Jul 23, 2020

glemieux approved these changes Jul 23, 2020

View reviewed changes

Merge pull request #2 from glemieux/lightning_v2_ctsm-externals

4bed6e2

Updating externals_clm configuration to new tag

rgknox approved these changes Jul 23, 2020

View reviewed changes

glemieux merged commit 82b67d1 into ESCOMP:fates_next_api Jul 23, 2020

glemieux mentioned this pull request Jul 24, 2020

Pre-merge deconflict ckoven/ctsm#2

Merged

glemieux mentioned this pull request Aug 25, 2020

CLMBuildNameList error for fates_main_api regression testmod #1125

Closed

glemieux mentioned this pull request Nov 12, 2022

Develop draft design document for elm-fates fire updates glemieux/E3SM#8

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend CNFireMethodMod with a FATESFireData class #991

Extend CNFireMethodMod with a FATESFireData class #991

slevis-lmwg commented Apr 24, 2020

ekluzek left a comment

ekluzek Apr 26, 2020

slevis-lmwg Apr 26, 2020

slevis-lmwg commented Apr 27, 2020

ekluzek left a comment

ekluzek commented May 4, 2020

ekluzek commented May 8, 2020

slevis-lmwg commented May 8, 2020

ekluzek commented May 8, 2020

slevis-lmwg commented May 8, 2020

slevis-lmwg Jul 12, 2020

slevis-lmwg commented Jul 12, 2020

glemieux commented Jul 13, 2020

slevis-lmwg commented Jul 14, 2020

glemieux commented Jul 14, 2020

jkshuman left a comment

jkshuman Jul 14, 2020

slevis-lmwg Jul 14, 2020

jkshuman Jul 14, 2020

slevis-lmwg Jul 14, 2020

slevis-lmwg Jul 14, 2020

jkshuman Jul 14, 2020

slevis-lmwg Jul 18, 2020

jkshuman Jul 14, 2020

slevis-lmwg Jul 14, 2020

slevis-lmwg Jul 18, 2020 •

edited

Loading

slevis-lmwg Jul 20, 2020

slevis-lmwg commented Jul 14, 2020

slevis-lmwg commented Jul 17, 2020

glemieux commented Jul 17, 2020

slevis-lmwg commented Jul 20, 2020

	! see bld/namelist_files/namelist_definition_clm4_5.xml for details
	! see bld/namelist_files/namelist_definition_clm4_5.xml for details
	! ignitions: 1=constant, 2=lightning, 3=obs, 4=lightning & anthro

	integer, public :: fates_spitfire_mode = 0 ! > 0 => use spitfire model: see bld/namelist_files/namelist_definition_clm4_5.xml for details
	integer, public :: fates_spitfire_mode = 0 ! > 0 => use spitfire model: ignitions: 1=constant, 2=lightning, 3=obs, 4=lightning & anthro

Extend CNFireMethodMod with a FATESFireData class #991

Extend CNFireMethodMod with a FATESFireData class #991

Conversation

slevis-lmwg commented Apr 24, 2020

Description of changes

Specific notes

ekluzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slevis-lmwg commented Apr 27, 2020

ekluzek left a comment

Choose a reason for hiding this comment

ekluzek commented May 4, 2020

ekluzek commented May 8, 2020

slevis-lmwg commented May 8, 2020

ekluzek commented May 8, 2020

slevis-lmwg commented May 8, 2020

Choose a reason for hiding this comment

slevis-lmwg commented Jul 12, 2020

glemieux commented Jul 13, 2020

slevis-lmwg commented Jul 14, 2020

glemieux commented Jul 14, 2020

jkshuman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slevis-lmwg Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slevis-lmwg commented Jul 14, 2020

slevis-lmwg commented Jul 17, 2020

glemieux commented Jul 17, 2020

slevis-lmwg commented Jul 20, 2020

slevis-lmwg Jul 18, 2020 •

edited

Loading