search

NOAA-OCM / QNSPECT

QGIS Plugin for NOAA Nonpoint Source Pollution and Erosion Comparison Tool (NSPECT)
GNU General Public License v2.0
7 stars 5 forks source link

Validate user input for Null Values in Lookup Table and return user friendly error #78

Open jamie-carter opened 2 years ago

jamie-carter commented 2 years ago

Latest QNSPECT version

Similar issue do not exist

What is the bug or the crash?

Null values in LC lookup causes the tool to fail. Perhaps include some language in the tool help and documentation that each cell needs a value

Steps to reproduce the issue

  1. Create a modified LC lookup table with NULL values in place of valid pollutant coefficients
  2. Run the Pollution Analysis tool

Versions

QGIS version 3.22.3-Białowieża QGIS code revision 1628765ec7 Qt version 5.15.2 Python version 3.9.5 GDAL/OGR version 3.4.1 PROJ version 8.2.1 EPSG Registry database version v10.041 (2021-12-03) GEOS version 3.10.0-CAPI-1.16.0 SQLite version 3.35.2 PDAL version 2.3.0 PostgreSQL client version 13.0 SpatiaLite version 5.0.1 QWT version 6.1.3 QScintilla2 version 2.11.5 OS version Windows 10 Version 1909

Active Python plugins curve_number_generator 1.3 mapswipetool_plugin 1.2 QNSPECT 0.1 db_manager 0.1.20 grassprovider 2.12.99 MetaSearch 0.3.5 processing 2.12.99 sagaprovider 2.12.99

Additional context

No response

ar-siddiqui commented 2 years ago

@jamie-carter I was not able to reproduce this issue.

  1. I changed the CSV file and didn't provide most values for nitrogen, yet the tool didn't fail; it assumed 0 for those Land Cover classes.
  2. I did the same in QGIS, created a lookup table using Create Lookup Table Template and changed nitrogen values to Null. The tool worked as in the previous case, assuming 0 value for them. image
DaveEslinger commented 2 years ago

@ar-siddiqui I can recreate the problem when using a coefficient table that is saved to a CSV file. If using either a temporary table (edits saved or unsaved) or a table in a GeoPackage (just tested with edits saved) the program works with Nulls.

It fails using a CSV table if the table contains either NULLs, or if the cells are completely empty.

Note that deleting the contents of a temporary or gpkg table immediately fills the cell with NULL. Deleting a CSV cell leaves it empty. You need to manually type in NULL to fill with a NULL. Clearly QGIS is handling these tables differently. All the above comments refer to modifying the table from within QGIS by opening the Attribute Table.

Bad CSV file and log of failing run attached. ccap_w_nulls.csv ccap_w_empty_cells.csv ccap_w_nulls_log.txt ccap_w_empty_cells_log.txt

ar-siddiqui commented 2 years ago

@DaveEslinger, thanks for the detailed feedback. I have looked into it and can recreate the issue.

After investigating, I think the user should not have empty/null values in the lookup table. In some of the cases, depending on the file format QGIS would assume null/empty values to be zeros and not in other cases and would fail (for example csv). This might be by design where certain file formats have their default null fallback values defined, while csv does not have it.

I think we should close this issue and ask users not to leave cells empty. To be more specific about where the issue is occurring I have added a print statement for each process so that the user can easily debug where the error is originating from.

image

DaveEslinger commented 2 years ago

I agree users should not have null values in their tables. However, I think we need to have a check for that and send an error message and stop processing if that occurs. Just telling them to use good input is unlikely to actually work. Input validation is needed.

jimparker99 commented 2 years ago

3 options discussed:

  1. Leave as-is. We guide users towards using CSVs with 0s and not nulls in documentation an our template. We accept some users will still introduce nulls.
  2. Check for CSV inputs only (not all supported formats) and then check for null values. Show specific error message and cancel the run. We probably need to check behavior of all supported formats when they contain nulls.
  3. Check inputs for nulls regardless of format, show error message on null and cancel the run. This would cancel runs that otherwise would have succeeded since some formats e.g. geopackage work with nulls values.

Decision for now is to stick with 1, until other must-have defects are addressed then revisit if there's time.