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.

Sign up for GitHub

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

Jump to bottom

Consolidate text between ARGS SPEC and UG (part 1 of 3) #604

Merged
merged 25 commits into from
Sep 13, 2021
Merged

Consolidate text between ARGS SPEC and UG (part 1 of 3) #604

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b8861ae
consolidate and rewrite about text for carbon and cbc models
emlys Jul 16, 2021
9be95bc
draft of CV changes
emlys Jul 17, 2021
d2d8048
Merge branch 'main' of https://github.com/natcap/invest into task/567…
emlys Aug 3, 2021
c7c382d
Merge branch 'release/3.10' of https://github.com/natcap/invest into …
emlys Aug 3, 2021
239830f
consolidate delineateit
emlys Aug 4, 2021
a05b44a
use format string for 'required if...' phrases
emlys Aug 4, 2021
79a5e12
Merge branch 'release/3.10' into task/567/models-A-D
emlys Aug 6, 2021
4b48893
crop production models
emlys Aug 6, 2021
bc88214
more
emlys Aug 17, 2021
6d99c86
Merge branch 'release/3.10' into task/567/models-A-D
emlys Aug 19, 2021
d42c8dc
add punctuation to about strings
emlys Aug 19, 2021
6ef5ecb
minor clean up
emlys Aug 19, 2021
e5a2ffb
more small changes to wording
emlys Aug 25, 2021
b40c8dc
Update src/natcap/invest/coastal_vulnerability.py
emlys Aug 30, 2021
461e34c
Update src/natcap/invest/spec_utils.py
emlys Aug 30, 2021
19d237e
changes from pr review
emlys Aug 30, 2021
c75c5c3
Merge branch 'release/3.10' into task/567/models-A-D
emlys Aug 31, 2021
1e919a4
revert mistakes
emlys Aug 31, 2021
d8f0c91
Merge branch 'task/567/models-A-D' of https://github.com/emlys/invest…
emlys Aug 31, 2021
9bb20b8
remove f string variables
emlys Sep 3, 2021
ba4b3de
clean up
emlys Sep 3, 2021
c1c5766
fix broken tests
emlys Sep 7, 2021
c6178e3
update option_string validation to cast value to string
emlys Sep 7, 2021
e960003
rephrase CBC landcover transitions table description
emlys Sep 8, 2021
639b2ee
add upper bound to gdal dependency
emlys Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 77 additions & 47 deletions src/natcap/invest/carbon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@
from . import validation
from . import utils
from . import spec_utils
from .spec_utils import u
from .spec_utils import u, REQUIRED_IF_SELECTED, RASTER_VALUES

LOGGER = logging.getLogger(__name__)


ARGS_SPEC = {
"model_name": "InVEST Carbon Model",
"module": __name__,
Expand All @@ -32,109 +33,144 @@
"lulc_cur_path": {
**spec_utils.LULC,
"projected": True,
"projection_units": u.meter,
"about": (
"A GDAL-supported raster representing the land-cover of the "
"current scenario."),
"name": "Current Land Use/Land Cover"
"A map of land cover for the current scenario. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should land cover be land use/land cover as you've used consistently below? I also might just be going too far with thinking how consistent these should be...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going with LULC everywhere for now

f"{RASTER_VALUES % 'Carbon Pools'}"),
"name": "current land use/land cover"
},
"calc_sequestration": {
"type": "boolean",
"required": "do_valuation | do_redd",
"about": (
"Check to enable sequestration analysis. This requires inputs "
"of Land Use/Land Cover maps for both current and future "
"scenarios."),
"name": "Calculate Sequestration"
"Enable sequestration analysis. This requires inputs "
"of land use/land cover maps for both current and future "
"scenarios. ") + REQUIRED_IF_SELECTED % (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate trying to unify the language for these common phrases. Is there a code style preference of concatenation like you have here or using f strings like above on line 39? Or, whatever looks most readable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to use f strings as much as possible but this one wouldn't all fit in one line

'REDD scenario analysis or run valuation model'),
"name": "calculate sequestration"
},
"lulc_fut_path": {
**spec_utils.LULC,
"projected": True,
"projection_units": u.meter,
"required": "calc_sequestration",
"about": (
"A GDAL-supported raster representing the land-cover of the "
"future scenario. If REDD scenario analysis is enabled, this "
"should be the reference, or baseline, future scenario "
"against which to compare the REDD policy scenario."),
"name": "Future Landcover"
"A map of land cover for the future scenario. "
f"{RASTER_VALUES % 'Carbon Pools'} If run valuation model is "
"selected, this should be the reference, or baseline, future "
"scenario against which to compare the REDD policy scenario. "
f"{REQUIRED_IF_SELECTED % 'calculate sequestration'}"),
"name": "future land use/land cover"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost wonder if we should have a LULC constant similar to the others for land use/ land cover. I'm always writing that in different ways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe because capitalization could make this tricky if it's going as the first thing in a sentence, we could have a document with commonly used terms to reference when writing new models. Sounds a bit tedious, but I'd love to not have to think about whether I should write: LULC, or land use/land cover or land use land cover, etc...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer a styleguide as the solution to this problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost wonder if we should have a LULC constant similar to the others

I like this

One advantage of a constant over a style guide is reducing repeated text - will reduce the number of words we need to get translated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, it probably won't help with translation. We can swap out translated text at the paragraph level, and probably sentences, but words/phrases need to be translated in context. So the benefit of using a constant is really just for consistency.

},
"do_redd": {
"type": "boolean",
"required": False,
"about": (
"Check to enable REDD scenario analysis. This requires three "
"Land Use/Land Cover maps: one for the current scenario, one "
"Enable REDD scenario analysis. This requires three "
"land use/land cover maps: one for the current scenario, one "
"for the future baseline scenario, and one for the future "
"REDD policy scenario."),
"name": "REDD Scenario Analysis"
"name": "REDD scenario analysis"
},
"lulc_redd_path": {
**spec_utils.LULC,
"projected": True,
"projection_units": u.meter,
"required": "do_redd",
"about": (
"A GDAL-supported raster representing the land-cover of the "
"REDD policy future scenario. This scenario will be compared "
"to the baseline future scenario."),
"name": "REDD Policy"
"A map of land cover for the REDD policy scenario. "
f"{RASTER_VALUES % 'Carbon Pools'} "
f"{REQUIRED_IF_SELECTED % 'REDD scenario analysis'}"),
"name": "REDD land use/land cover"
},
"carbon_pools_path": {
"type": "csv",
"columns": {
"lucode": {"type": "integer"},
"c_above": {"type": "number", "units": u.metric_ton/u.hectare},
"c_below": {"type": "number", "units": u.metric_ton/u.hectare},
"c_soil": {"type": "number", "units": u.metric_ton/u.hectare},
"c_dead": {"type": "number", "units": u.metric_ton/u.hectare}
"lucode": {
"type": "integer",
"about": (
"Land use/land cover code. Every value in the LULC "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use LULC here? I think it's the first time it's been used to describe land use/land cover. Again, I'm probably taking this too far, haha

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the 'spell out the first time' rule in this context may not be possible because the order of displaying these strings in the UG is not guaranteed to be the same. I think for common acronyms (LULC, DEM, AOI, TFA) it should be ok to define them only once for the whole user's guide (like on the data sources page). But also, sometimes it's helpful to write out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for me, TFA is not like those others in terms of the recognizability of the acronym.

"maps must have a corresponding entry in this column.")
},
"c_above": {
"type": "number",
"units": u.metric_ton/u.hectare,
"about": "Carbon density of aboveground biomass."},
"c_below": {
"type": "number",
"units": u.metric_ton/u.hectare,
"about": "Carbon density of belowground biomass."},
"c_soil": {
"type": "number",
"units": u.metric_ton/u.hectare,
"about": "Carbon density of soil."},
"c_dead": {
"type": "number",
"units": u.metric_ton/u.hectare,
"about": "Carbon density of dead matter."}
},
"about": (
"A table that maps the each LULC class from the LULC map(s)to "
"the amount of carbon in their carbon pools."),
"name": "Carbon Pools"
"A table that maps each LULC code to carbon pool data for "
"that LULC type."),
dcdenu4 marked this conversation as resolved.
Show resolved Hide resolved
"name": "carbon pools"
},
"lulc_cur_year": {
"expression": "float(value).is_integer()",
"type": "number",
"units": u.year,
"required": "do_valuation",
"about": "The calendar year of the current scenario.",
"name": "Current Land Cover Calendar Year"
"about": (
"The calendar year of the current scenario depicted in the "
"current land use/land cover. "
f"{REQUIRED_IF_SELECTED % 'run valuation model'}"),
"name": "current land cover year"
},
"lulc_fut_year": {
"expression": "float(value).is_integer()",
"type": "number",
"units": u.year,
"required": "do_valuation",
"about": "The calendar year of the future scenario.",
"name": "Future Land Cover Calendar Year"
"about": (
"The calendar year of the future scenario depicted in the "
"future land use/land cover map. "
f"{REQUIRED_IF_SELECTED % 'run valuation model'}"),
"name": "future land cover year"
},
"do_valuation": {
"type": "boolean",
"required": False,
"about": (
"Calculate NPV for a future or REDD scenario "
"and report in final HTML document."),
"name": "Run Valuation Model"
"Calculate net present value for the future scenario, and the "
"REDD scenario if provided, and report it in the final HTML "
"document."),
"name": "run valuation model"
},
"price_per_metric_ton_of_c": {
"type": "number",
"units": u.currency/u.ton,
"required": "do_valuation",
"about": "The present value of carbon per metric ton.",
"about": (
"The present value of carbon. "
f"{REQUIRED_IF_SELECTED % 'run valuation model'}"),
"name": "price of carbon"
},
"discount_rate": {
"type": "ratio",
"required": "do_valuation",
"about": "The discount rate as a floating point percent.",
"name": "Market Discount in Price of Carbon (%)"
"about": (
"The annual market discount rate in the price of carbon, "
"which reflects society's preference for immediate benefits "
"over future benefits. "
f"{REQUIRED_IF_SELECTED % 'run valuation model'}"),
"name": "annual market discount rate"
},
"rate_change": {
"type": "ratio",
"required": "do_valuation",
"about": (
"The floating point percent increase of the price of carbon "
"per year."),
"name": "Annual Rate of Change in Price of Carbon"
"The annual increase of the price of carbon. "
f"{REQUIRED_IF_SELECTED % 'run valuation model'}"),
"name": "annual price change"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this name or the about section get at this being a rate of change versus absolute change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to "The relative annual increase". Hopefully that plus the type (ratio) will make it clear enough?

}
}
}
Expand Down Expand Up @@ -165,12 +201,6 @@
'c_dead_redd': 'c_dead_redd.tif',
}

_TMP_BASE_FILES = {
'aligned_lulc_cur_path': 'aligned_lulc_cur.tif',
'aligned_lulc_fut_path': 'aligned_lulc_fut.tif',
'aligned_lulc_redd_path': 'aligned_lulc_redd.tif',
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these files unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was wrong about this, not sure why I removed it.

# -1.0 since carbon stocks are 0 or greater
_CARBON_NODATA = -1.0
# use min float32 which is unlikely value to see in a NPV raster
Expand Down
Loading