natcap / invest

InVEST®: models that map and value the goods and services from nature that sustain and fulfill human life.
Apache License 2.0
152 stars 64 forks source link

UNA: Population group SUP_DEM outputs missing in admin gpkg #1531

Closed newtpatrol closed 2 months ago

newtpatrol commented 5 months ago

I'm running 3.14.1 with the Aggregate by Population Groups option (not radii by population group). It runs successfully, and according to the User Guide, there should be 3 fields added to admin_boundaries.gpkg corresponding with each population group. Pund_adm and Povr_adm are added for each population group, but SUP_DEMadm_cap is not added per population group, the only SUP_DEM column is for the total population. This seems like a bug.

Here's the datastack I'm using that produces this result (bummer that GitHub doesn't support .tgzs for direct upload).

emlys commented 3 months ago

Confirmed this is a bug.

emlys commented 3 months ago

After spending all day trying to understand the different UNA modes, I'm still not totally sure what the answer is.

As far as I can tell, the model design doc does not specify a SUP_DEM result per population group when run in "aggregate by population groups" mode.

This kind of makes sense, because the SUP_DEM per capita results would be the same for all groups:

Intuitively, because everyone has the same search radius, the urban nature supply and demand is the same for everyone living on a given pixel.

But the people living on each pixel may disproportionately belong to one group or another. Thus, each group may disproportionately experience under- or over-supply of urban nature. We report that as $P{und}$ and $P{ovr}$ (units: number of people).

Conceptually, I think you could report a total SUP_DEM by population group (units: m^2). We already have

So I think you could also do

But that is not part of the original specification, and I'm not sure that it adds much value to an already confusing model.

phargogh commented 2 months ago

I have been going back and forth on this, but at this point I wonder if it might just be easier to implement the as-yet-unimplemented trivial calculations for urban nature balance for pop groups when aggregating by pop groups even though it doesn't add much interesting information.

The root bug here is that I, for some reason I don't recall, thoughy it would be a good idea to allow the user to aggregate by population groups when not running the model with search radii defined per population group.

W/r/t what to do about this issue, I agree that this model is ridiculously complicated for how simple its operations are, but I also wonder about the time required to make the model fully compliant with the original spec.

@emlys would you agree that a code patch for this would be as simple as the below (not counting needed changes in the UG)?

diff --git a/src/natcap/invest/urban_nature_access.py b/src/natcap/invest/urban_nature_access.py
index 2831e6528..5f3b2345e 100644
--- a/src/natcap/invest/urban_nature_access.py
+++ b/src/natcap/invest/urban_nature_access.py
@@ -2074,6 +2074,8 @@ def _supply_demand_vector_for_single_raster_modes(
                                          ('Povr', oversupplied_stats)]:
                 stats[f'{prefix}_adm_{group}'] = (
                     supply_stats[fid]['sum'] * group_proportion)
+            stats[f'SUP_DEMadm_cap_{group}'] = (
+                per_capita_supply * group_proportion)
         stats_by_feature[fid] = stats

     _write_supply_demand_vector(
newtpatrol commented 2 months ago

For what it's worth, seeing @emlys very helpful evaluation, and thinking about it more, I agree that it doesn't make sense to include a SUP_DEM result per population group when not running with different radii per population group, since the result is per capita, and will be the same as the general SUP_DEM result.

I do think it's good to provide the over- and under-supply by population group, since those will change based on the proportion of different groups, even without differing radii, and definitely provides useful information related to equity.

Maybe I'm over-simplifying, but my suggestion is not to add any other model outputs, but just update the User Guide to correspond with the current outputs, and add explanatory text about SUP_DEM being the same in this case. I'd be happy to make these updates if that's helpful.

emlys commented 2 months ago

@newtpatrol we discussed this today on the software team call and agree with leaving the model as-is, and just updating the user guide.

phargogh commented 2 months ago

The UG has been updated to address this issue, so I believe we can now close it.