Repository: natcap/invest (python)

Issue:
Carbon: UI validation and grouping for year inputs
The LULC Calendar Year inputs are only need for the valuation component of the model, But the UI requires them for sequestration also, which is confusing because they are not used in the sequestration calcs.

Pull request:
Bugfix/carbon lulc year inputs should be tied to valuation
Fixes #541 

This PR moves the two Calendar Year inputs into the Valuation Container of the UI. Now, these inputs are required to "do valuation", but not required to "calc sequestration". The User's Guide and the model's execute function confirmed these args are only used when calculating valuation.

Also some small updates to 'ARGS_SPEC' "About" text for some other inputs.

Here's the new UI
![CarbonQtUI](https://user-images.githubusercontent.com/4071255/116752176-22161c00-a9d3-11eb-8e1d-f07a2a4c15da.PNG)


# Checklist
- [x] Updated HISTORY.rst (if these changes are user-facing)

- [x] Updated the user's guide (if needed)

The UG changes are in a PR also, just in case we don't go with this.  https://github.com/natcap/invest.users-guide/pull/26

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 7677aa68e..c6db4cb20 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -69,6 +69,8 @@ Unreleased Changes (3.9.1)
     * Fixed a bug where, if rate change and discount rate were set to 0, the
       valuation results were in $/year rather than $, too small by a factor of
       ``lulc_fut_year - lulc_cur_year``.
+    * Improved UI to indicate that Calendar Year inputs are only required for
+      valuation, not also for sequestration.
 * DelineateIt:
     * The DelineateIt UI has been updated so that the point-snapping options
       will always be interactive.
diff --git a/src/natcap/invest/carbon.py b/src/natcap/invest/carbon.py
index 118eeb709..c82fa17bc 100644
--- a/src/natcap/invest/carbon.py
+++ b/src/natcap/invest/carbon.py
@@ -85,17 +85,17 @@ ARGS_SPEC = {
         },
         "carbon_pools_path": {
             "validation_options": {
-                "required_fields": ["LUCODE", "C_above", "C_below", "C_soil",
+                "required_fields": ["lucode", "C_above", "C_below", "C_soil",
                                     "C_dead"],
             },
             "type": "csv",
             "required": True,
             "about": (
-                "A table that maps the land-cover IDs to carbon pools.  "
-                "The table must contain columns of 'LULC', 'C_above', "
-                "'C_Below', 'C_Soil', 'C_Dead' as described in the User's "
-                "Guide.  The values in LULC must at least include the LULC "
-                "IDs in the land cover maps."),
+                "A table that maps each land cover class to its respective "
+                "carbon pools. The table must contain columns of 'lucode', "
+                "'C_above', 'C_below', 'C_soil', 'C_dead' as described in the "
+                "User's Guide. The 'lucode' column must include all the pixel "
+                "values present in the land cover maps."),
             "name": "Carbon Pools"
         },
         "lulc_cur_year": {
@@ -103,36 +103,31 @@ ARGS_SPEC = {
                 "expression": "int(value)"
             },
             "type": "number",
-            "required": "calc_sequestration",
+            "required": "do_valuation",
             "about": "The calendar year of the current scenario.",
-            "name": "Current Landcover Calendar Year"
+            "name": "Current Land Cover Calendar Year"
         },
         "lulc_fut_year": {
             "validation_options": {
                 "expression": "int(value)"
             },
             "type": "number",
-            "required": "calc_sequestration",
+            "required": "do_valuation",
             "about": "The calendar year of the future scenario.",
-            "name": "Future Landcover Calendar Year"
+            "name": "Future Land Cover Calendar Year"
         },
         "do_valuation": {
             "type": "boolean",
             "required": False,
             "about": (
-                "if true then run the valuation model on available outputs.  "
-                "At a minimum will run on carbon stocks, if sequestration "
-                "with a future scenario is done and/or a REDD scenario "
-                "calculate NPV for either and report in final HTML "
-                "document."),
+                "Calculate NPV for a future or REDD scenario "
+                "and report in final HTML document."),
             "name": "Run Valuation Model"
         },
         "price_per_metric_ton_of_c": {
             "type": "number",
             "required": "do_valuation",
-            "about": (
-                "Is the present value of carbon per metric ton. Used if "
-                "``args['do_valuation']`` is present and True."),
+            "about": "The present value of carbon per metric ton.",
             "name": "Price/Metric ton of carbon"
         },
         "discount_rate": {
@@ -220,17 +215,15 @@ def execute(args):
             storage density to lulc codes. (required if 'do_uncertainty' is
             false)
         args['lulc_cur_year'] (int/string): an integer representing the year
-            of `args['lulc_cur_path']` used if `args['calc_sequestration']`
+            of `args['lulc_cur_path']` used if `args['do_valuation']`
             is True.
         args['lulc_fut_year'](int/string): an integer representing the year
             of `args['lulc_fut_path']` used in valuation if it exists.
             Required if  `args['do_valuation']` is True and
             `args['lulc_fut_path']` is present and well defined.
         args['do_valuation'] (bool): if true then run the valuation model on
-            available outputs.  At a minimum will run on carbon stocks, if
-            sequestration with a future scenario is done and/or a REDD
-            scenario calculate NPV for either and report in final HTML
-            document.
+            available outputs. Calculate NPV for a future scenario or a REDD
+            scenario and report in final HTML document.
         args['price_per_metric_ton_of_c'] (float): Is the present value of
             carbon per metric ton. Used if `args['do_valuation']` is present
             and True.
diff --git a/src/natcap/invest/ui/carbon.py b/src/natcap/invest/ui/carbon.py
index ebb81263d..4f1afffe3 100644
--- a/src/natcap/invest/ui/carbon.py
+++ b/src/natcap/invest/ui/carbon.py
@@ -31,16 +31,9 @@ class Carbon(model.InVESTModel):
             label='Carbon Pools',
             validator=self.validator)
         self.add_input(self.carbon_pools_path)
-        self.cur_lulc_year = inputs.Text(
-            args_key='lulc_cur_year',
-            helptext='The calendar year of the current scenario.',
-            interactive=False,
-            label='Current Landcover Calendar Year',
-            validator=self.validator)
-        self.add_input(self.cur_lulc_year)
         self.calc_sequestration = inputs.Checkbox(
             helptext=(
-                "Check to enable sequestration analysis.  This "
+                "Check to enable sequestration analysis. This "
                 "requires inputs of Land Use/Land Cover maps for both "
                 "current and future scenarios."),
             args_key='calc_sequestration',
@@ -55,16 +48,9 @@ class Carbon(model.InVESTModel):
                 "baseline, future scenario against which to compare "
                 "the REDD policy scenario."),
             interactive=False,
-            label='Future Landcover (Raster)',
+            label='Future Land Cover (Raster)',
             validator=self.validator)
         self.add_input(self.fut_lulc_raster)
-        self.fut_lulc_year = inputs.Text(
-            args_key='lulc_fut_year',
-            helptext='The calendar year of the future scenario.',
-            interactive=False,
-            label='Future Landcover Calendar Year',
-            validator=self.validator)
-        self.add_input(self.fut_lulc_year)
         self.redd = inputs.Checkbox(
             helptext=(
                 "Check to enable REDD scenario analysis.  This "
@@ -93,9 +79,23 @@ class Carbon(model.InVESTModel):
             interactive=False,
             label='Run Valuation Model')
         self.add_input(self.valuation_container)
+        self.cur_lulc_year = inputs.Text(
+            args_key='lulc_cur_year',
+            helptext='The calendar year of the current scenario.',
+            interactive=False,
+            label='Current Land Cover Calendar Year',
+            validator=self.validator)
+        self.valuation_container.add_input(self.cur_lulc_year)
+        self.fut_lulc_year = inputs.Text(
+            args_key='lulc_fut_year',
+            helptext='The calendar year of the future scenario.',
+            interactive=False,
+            label='Future Land Cover Calendar Year',
+            validator=self.validator)
+        self.valuation_container.add_input(self.fut_lulc_year)
         self.price_per_metric_ton_of_c = inputs.Text(
             args_key='price_per_metric_ton_of_c',
-            label='Price/Metric ton of carbon',
+            label='Price/Metric Ton of Carbon',
             validator=self.validator)
         self.valuation_container.add_input(self.price_per_metric_ton_of_c)
         self.discount_rate = inputs.Text(
@@ -114,12 +114,8 @@ class Carbon(model.InVESTModel):
         self.valuation_container.add_input(self.rate_change)
 
         # Set interactivity, requirement as input sufficiency changes
-        self.calc_sequestration.sufficiency_changed.connect(
-            self.cur_lulc_year.set_interactive)
         self.calc_sequestration.sufficiency_changed.connect(
             self.fut_lulc_raster.set_interactive)
-        self.calc_sequestration.sufficiency_changed.connect(
-            self.fut_lulc_year.set_interactive)
         self.calc_sequestration.sufficiency_changed.connect(
             self.redd.set_interactive)
         self.redd.sufficiency_changed.connect(


Test patch:
diff --git a/tests/test_carbon.py b/tests/test_carbon.py
index 65f3a2589..75a657033 100644
--- a/tests/test_carbon.py
+++ b/tests/test_carbon.py
@@ -301,9 +301,7 @@ class CarbonValidationTests(unittest.TestCase):
         invalid_keys = validation.get_invalid_keys(validation_errors)
         expected_missing_keys = set(
             self.base_required_keys +
-            ['lulc_cur_year',
-             'lulc_fut_year',
-             'lulc_fut_path'])
+            ['lulc_fut_path'])
         self.assertEqual(invalid_keys, expected_missing_keys)
 
     def test_missing_keys_redd(self):
@@ -333,5 +331,7 @@ class CarbonValidationTests(unittest.TestCase):
             ['calc_sequestration',
              'price_per_metric_ton_of_c',
              'discount_rate',
-             'rate_change'])
+             'rate_change',
+             'lulc_cur_year',
+             'lulc_fut_year'])
         self.assertEqual(invalid_keys, expected_missing_keys)