Convert per-pixel values to per-hectare values in raster outputs of multiple models #1717
Conversation
…e/1270-per-hectare-outputs
There was a problem hiding this comment.
This looks great, @emilyanndavis. I appreciate the comments about unit conversions and using the same regression values when possible to confirm that the results are correct. I have just a few minor comments.
| @@ -548,10 +548,8 @@ def _generate_carbon_map( | |||
| Returns: | |||
| None. | |||
| """ | |||
There was a problem hiding this comment.
I couldn't comment on the line, but the out_carbon_stock_path docstring might need updating for the new units.
| @@ -835,9 +832,8 @@ def execute(args): | |||
| args=(args['aggregate_polygon_path'], | |||
| target_aggregate_vector_path, | |||
| landcover_raster_info['projection_wkt'], | |||
| crop_names, nutrient_df, | |||
| output_dir, file_suffix, | |||
| aggregate_results_table_path), | |||
There was a problem hiding this comment.
I see aggregate_results_table_path was also being defined within the function, so it appears redundant to pass it in here. But for the sake of taskgraph and knowing the complete target_path_list, should we continue passing this filepath as an arg?
There was a problem hiding this comment.
Good question. Looking back at this now, I'm sure I must have removed aggregate_results_table_path from the args because it wasn't being referenced by the aggregate_regression_results_to_polygons function. It is still included in the target_path_list that is passed to taskgraph, though, and the target_path_list does get logged (at least when taskgraph logging is set to debug). Would you say that's sufficient, or do you think I should add aggregate_results_table_path back to the args as well?
src/natcap/invest/sdr/sdr.py
Outdated
| @@ -188,7 +188,7 @@ | |||
| "about": "Total potential soil loss per pixel in the original land cover calculated from the USLE equation. (Eq. (68))", | |||
tests/test_ndr.py
Outdated
| @@ -247,6 +251,27 @@ def test_base_regression(self): | |||
| args['workspace_dir'], 'intermediate_outputs', | |||
| 'what_drains_to_stream.tif'))) | |||
|
|
|||
| # Check raster outputs to make sure values are in kg/ha/yr. | |||
| # Pixel size in test data is 30m x 30m = 900 m^2. | |||
| pixels_per_hectare = 10000 / 900 | |||
There was a problem hiding this comment.
Does it make sense to get the pixel size directly from the test data instead of hardcoding it? It might make this a bit more resilient to future changes.
There was a problem hiding this comment.
Good idea, thanks!
tests/test_sdr.py
Outdated
| @@ -175,6 +177,28 @@ def test_base_regression(self): | |||
| self.assertEqual( | |||
| numpy.count_nonzero(sed_dep_array[negative_non_nodata_mask]), 0) | |||
|
|
|||
| # Check raster outputs to make sure values are in tons/ha/yr. | |||
| # Pixel size in test data is 50m x 50m = 2500 m^2. | |||
| pixels_per_hectare = 10000 / 2500 | |||
There was a problem hiding this comment.
Same comment here - should we get the pixel size directly from the raster?
Description
Fixes #1270. Specifically, the following models (and their tests) have been updated:
Where these models previously produced raster outputs whose pixel values were in per-pixel units (e.g., Mg C/px, or kg N/(px•yr), etc.), they now produce raster outputs whose pixel values are in per-hectare units (e.g., Mg C/ha, or kg N/(ha•yr), etc.).
Regression tests now check raster outputs to make sure their values are in the appropriate units, and they also check any summary tables to make sure reported totals (e.g., total Mg C, or total kg N/yr, etc.) remain unchanged.
(Also includes some cleanup of miscellaneous items, such as unused variables, PEP8 issues picked up by my linter, and typos I noticed while reading the code and/or the user guide.)
Checklist
[ ] Tested the Workbench UI (if relevant)