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

Validation should by default require nodata values to be defined #226

Closed
phargogh opened this issue Jul 14, 2020 · 3 comments
Closed

Validation should by default require nodata values to be defined #226

phargogh opened this issue Jul 14, 2020 · 3 comments
Assignees
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@phargogh
Copy link
Member

phargogh commented Jul 14, 2020

There are many places in InVEST where we do a raster_calculator operation along these lines:

valid_pixels = numpy.isclose(array, nodata_value)

But if nodata_value=None:

>>> import numpy
>>> array = numpy.array([1]))
>>> valid_pixels = numpy.isclose(array, None)
TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

If the user might not have a defined nodata value, we need to guard against this possibility. Here's an issue on the forums where this came up and propagated to a very strange error much farther down the line. It would be preferable to preempt this error by simply validating for the presence of a defined nodata value for any raster that comes in to InVEST.

EDIT: in the forums example linked above, this particular issue was observed in a climate zones calculation in SWY. Other models will undoubtedly have this issue as well.

@phargogh phargogh added the bug Something isn't working label Jul 14, 2020
@dcdenu4
Copy link
Member

dcdenu4 commented Jul 14, 2020

I agree this is an issue that should be addressed. I think I implemented undefined no data validation in the HQ refactor and Rich made the point that having an undefined nodata value is valid and that perhaps the model or InVEST at large should be checking / handling these cases as opposed to not allowing them. Just wanted to bring it up here as a topic that has had some discussion in the past.

@phargogh phargogh self-assigned this Jul 14, 2020
@phargogh phargogh added the in progress This issue is actively being worked on label Jul 14, 2020
@phargogh
Copy link
Member Author

@dcdenu4 just to clarify, it sounds like the result of that conversation was that the models should individually handle the possibility that a nodata value is defined, is that right?

@phargogh phargogh removed the in progress This issue is actively being worked on label Jul 14, 2020
@phargogh
Copy link
Member Author

@dcdenu4 and I just chatted about this issue this morning and we agreed that rasters with undefined nodata values are, in fact, a valid configuration and should be handled appropriately in our models.

#228 has been created to track this issue.

@phargogh phargogh added the wontfix This will not be worked on label Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants