-
Notifications
You must be signed in to change notification settings - Fork 71
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
Consolidate text between ARGS SPEC and UG (part 1 of 3) #604
Conversation
src/natcap/invest/carbon.py
Outdated
"name": "Calculate Sequestration" | ||
"Enable sequestration analysis. This requires inputs " | ||
"of land use/land cover maps for both current and future " | ||
"scenarios. ") + REQUIRED_IF_SELECTED % ( |
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.
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?
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.
I was trying to use f strings as much as possible but this one wouldn't all fit in one line
src/natcap/invest/carbon.py
Outdated
"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" |
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.
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.
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.
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...
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.
I think I prefer a styleguide as the solution to this problem.
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.
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.
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.
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.
src/natcap/invest/carbon.py
Outdated
"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. " |
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.
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...
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.
I'm going with LULC
everywhere for now
src/natcap/invest/carbon.py
Outdated
"lucode": { | ||
"type": "integer", | ||
"about": ( | ||
"Land use/land cover code. Every value in the LULC " |
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.
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
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.
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.
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.
At least for me, TFA is not like those others in terms of the recognizability of the acronym.
"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" |
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.
I'm not sure this name or the about section get at this being a rate of change versus absolute change.
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.
I changed this to "The relative annual increase". Hopefully that plus the type (ratio) will make it clear enough?
src/natcap/invest/carbon.py
Outdated
_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', | ||
} | ||
|
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.
Were these files unused?
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.
Yea
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.
Actually I was wrong about this, not sure why I removed it.
}, | ||
"do_economic_analysis": { | ||
"name": "Calculate Net Present Value of Sequestered Carbon", | ||
"name": "Do Valuation", |
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.
I think this name should more reflect the valuation being done at the cost of being a little wordy. Maybe I just don't like the Do
part. Calculate Valuation
, Run Valuation
...
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.
Changed to Run Valuation
"The price of CO2E at the baseline year. Required if Do " | ||
"Valuation is selected and Use Price Table is not selected."), |
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.
Should this use the REQUIRED_IF_SELECTED
?
f"{REQUIRED_IF_SELECTED % 'Do Valuation'} and Use Price Table is not selected".
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.
Right now the REQUIRED_IF_SELECTED
is a standalone sentence ending with a period, so it wouldn't work exactly here.
tests/test_args_specs.py
Outdated
if any(l.isupper() for l in arg['name']): | ||
print(arg['name']) |
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.
Was this for debug purposes?
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.
yea, I was making sure to lowercase all the names. Removed this.
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.
Hey @emlys , this is looking great. Also a shout out to whoever is updating the Args Spec wiki portion with these changes / ideas in mind. My first thought was, we should make sure to get this down in the wiki, and it already looks like it's there.
A few things came to mind when reviewing.
- Do we have formal guidelines on
LULC
vsland use/land cover
vsland cover
? - How do others feel about the
REQUIRED_IF_SELECTED
helper for consistent phrasing? I think I like this approach but do think the only benefit is consistency, since it doesn't really shorten anything and is more syntax onerous. - Side-Note: Should we update the
args
section of the wiki to better define what information should go in that docstring?
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.
src/natcap/invest/carbon.py
Outdated
"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" |
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.
I think I prefer a styleguide as the solution to this problem.
"Annual increase in the price of CO2E. Required if Do " | ||
"Valuation is selected and Use Price Table is not selected.") |
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.
I can imagine that when editing a bulk of these it came in handy. But going forward, adding one or two new specs 6 months or a year from now, I think it will be easier for me to write the whole sentence here. Also easier to verify that it's correct grammar.
I don't feel too strongly so I'm okay with leaving these in. I'm just not sure I'd prefer to use them myself in the future.
"about": ( | ||
"Path to a polygon vector that is projected in a coordinate " | ||
"system with units of meters. The polygon should intersect " | ||
"the landmass and the shelf contour line"), |
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.
Thanks, this bit about the shelf contour line is outdated and not true anymore.
}, | ||
"model_resolution": { | ||
"type": "number", | ||
"expression": "value > 0", | ||
"units": u.meter, | ||
"about": ( | ||
"Distance at which the coastline will be resolved. Coastline " | ||
"features smaller than this distance will not be represented " | ||
"by the shoreline points. Points will be spaced at intervals " |
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.
Thanks again, this bit about small coastline features not being represented isn't accurate either, though maybe it should be.
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.
Oh, I just moved that phrase over to the RST side... I'll take it out completely.
"polygon because some functions in the model require " | ||
"searching for landmasses around shore points up to the " | ||
"distance defined in Maximum Fetch Distance, which likely " | ||
"extends beyond the AOI polygon."), |
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.
Beautiful
}, | ||
"dem_path": { | ||
"type": "raster", | ||
**spec_utils.DEM, | ||
"bands": {1: { | ||
"type": "number", | ||
"units": u.linear_unit # any unit of length is ok |
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.
I commented on the UG PR about omitting units: none
from the rendered docs. I sort of feel similarly here, where the docs "units: linear unit" is a point of confusion at worst and just ignored by the reader at best. Honestly, I think CV would work fine even if the DEM has height units in decimal degrees, which would be very weird.
Not sure what the solution is. It's slightly inaccurate to change the spec to u.none
. And I'm guessing a units
attribute is required for number
types? Any thoughts?
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.
If 'linear unit' is too confusing, I'll just go with u.none
. While any unit would work, you could even have a unitless "height index" and that would still work as long as higher points have bigger values.
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.
Sounds great to me
"2": "low", | ||
"3": "moderate", | ||
"4": "high", | ||
"5": "very high" |
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.
These descriptions need to be inverted actually. 5 is the rank for no habitat protection at all. 1 is for very high protection.
Or in other words, once the ranks are assigned to each shore point, 5 is very high exposure at a point, 1 is very low exposure. That matches how the other exposure variables are scaled.
But I think when preparing this CSV, it's more intuitive to think in terms of "protection offered". Can we invert them and add "protection" to each one for good measure?
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.
Oh okay, that makes more sense.
}, | ||
"about": ( | ||
"Relative amount of coastline protection this habitat " | ||
"provides.") |
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.
Exactly.
"3": "moderate", | ||
"4": "high", | ||
"5": "very high" | ||
}, |
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.
This one's perfect because it's in terms of exposure. If we want to we could again add "exposure" to the end of each description.
"Exposure rank to assign to any shore points that are not " | ||
"near to any segment in the geomorphology vector. " | ||
f"{REQUIRED_IF_PROVIDED % 'Geomorphology vector'}"), | ||
"name": "geomorphology fill value" | ||
}, | ||
"population_raster_path": { | ||
"type": "raster", | ||
"bands": { | ||
1: {"type": "number", "units": u.none} |
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.
I'm fine with leaving this as u.none
and letting the about section do the describing. But if there's a u.count
that would also be accurate.
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.
I'd prefer not to make the count
vs none
distinction, since I don't fully understand it
Co-authored-by: Doug <[email protected]>
Co-authored-by: Doug <[email protected]>
…into task/567/models-A-D
As we agreed in the coffee call this morning, I took out all the f-string variables and instead we'll follow a style guide for those things. |
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.
Hey @emlys , thanks for addressing my comments. I think this is looking good.
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.
Latest changes look good to me!
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.
Changes look good but a few tests are failing, I think related to these changes.
ReadTheDocs build is failing on GDAL install with this error: |
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.
Okay I approve! @emlys I'll let you decide whether to merge yet or hold out for anything else.
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.
Latest changes look great to me! I'll leave it up to you @emlys about the readthedocs build and/or if there's anything else before merging.
The setuptools issue only happened because I forgot to update the GDAL requirement in So we don't need to do anything about this, and I'm going to merge once the tests pass. Before: expected failure when the gdal library isn't already installed
Now: different failure due to setuptools change
Looks like they may have resolved this for gdal 3.3.1 also. |
703a796
to
639b2ee
Compare
Description
First part of #567.
Checklist
- [ ] Updated HISTORY.rst (if these changes are user-facing)- [ ] Updated the user's guide (if needed)