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.

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

UFRM: incorrect ValueError raised if biophysical table includes a row of all 0s #1123

Closed
davemfish opened this issue Nov 22, 2022 · 2 comments · Fixed by #1342
Closed

UFRM: incorrect ValueError raised if biophysical table includes a row of all 0s #1123

davemfish opened this issue Nov 22, 2022 · 2 comments · Fixed by #1342
Assignees
Labels
bug Something isn't working in progress This issue is actively being worked on
Milestone

Comments

@davemfish
Copy link
Contributor

davemfish commented Nov 22, 2022

This came up on this forum post.

As is somewhat common, a LULC raster without a nodata value was used. But the raster was filled with 0s that probably represent nodata (or maybe water), and so the biophysical table was given a row for lucode 0, filled with 0s for CN_A, CN_B, CN_C, CN_D.

I don't know the details of this model, and whether setting those parameters to 0 is equivalent to setting that pixel value to nodata, but I don't think it matters.

Here's the problem

This model does fancy things using a sparse matrix as a lookup table for these CN values. Such a matrix can appear to have a row full of 0s for two different reasons:

  1. Because the biophysical table had a row full of 0s (this should not raise an error, but does)
  2. Because a landuse code was missing from the biophysical table (this should raise an error, and does)
@davemfish davemfish added the bug Something isn't working label Nov 22, 2022
@davemfish davemfish added this to the 3.12.1 milestone Nov 22, 2022
@davemfish
Copy link
Contributor Author

Some context for this; this bug was created while fixing another one. #686 #698

@davemfish davemfish added the in progress This issue is actively being worked on label Nov 22, 2022
@davemfish davemfish self-assigned this Nov 22, 2022
@davemfish davemfish modified the milestones: 3.12.1, 3.12.2 Dec 2, 2022
@davemfish davemfish modified the milestones: 3.12.2, 3.13.1 Mar 17, 2023
@davemfish davemfish removed the in progress This issue is actively being worked on label Apr 7, 2023
@dcdenu4
Copy link
Member

dcdenu4 commented May 8, 2023

This came up again on the forums and I was writing a new issue when I discovered this one existed. Here's what I was writing:

I think we've mostly settled on being able to support raster inputs that do not have a defined nodata value because GDAL supports it. However, a lot of rasters that do not have a defined nodata often have a value "acting" as one. So, in a lot of our models this means adding a row to a biophysical table that accounts for that value which is "acting" as nodata but not defined as such. In the UFM model there is a situation where the model checks to make sure there aren't any LULC values not represented in the table as follows:

 # Even without an IndexError, still must guard against           
 # lucodes that can index into the sparse matrix but were         
 # missing from the biophysical table. They have rows of all 0.   
 if not cn_matrix.sum(1).all():                                   
     empty_rows = numpy.where(lucode_to_cn_table.sum(1) == 0)     
     missing_codes = numpy.intersect1d(valid_lucodes, empty_rows) 
     raise ValueError(                                            
         f'The biophysical table is missing a row for lucode(s) ' 
         f'{missing_codes.tolist()}')                             

This logic doesn't allow for a user entered row of all 0 values, which end up being caught as that rows lucode not being represented in the table, when it actually is.

Forum link: https://community.naturalcapitalproject.org/t/urban-flood-risk-mitigation-model-area-of-analysis/3583/8

@davemfish davemfish added the in progress This issue is actively being worked on label Jun 27, 2023
davemfish added a commit to davemfish/invest that referenced this issue Jun 27, 2023
davemfish added a commit to davemfish/invest that referenced this issue Jun 27, 2023
davemfish added a commit to davemfish/invest that referenced this issue Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in progress This issue is actively being worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants