search

natcap / invest

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

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

Closed phargogh closed 4 years ago

phargogh commented 4 years ago

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.

dcdenu4 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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.

https://github.com/natcap/invest/issues/228 has been created to track this issue.