usgs-review: components and documentation
The following are suggestions to consider related to package components and documentation. When possible I made direct suggestions.
Software Release Components
-
README.md --- I’ve been suggested in past to include software disclaimer text, DOI and IPDS numbers in Readme as well as disclaimer.md. A DOI is only required for scientific software so that may not apply. -
Disclaimer.md --- Add the appropriate approved or preliminary software disclaimer text from the USGS FSP website prior to finalizing released branch/tag. Assuming the master branch will remain a working branch, it and all other working branches will retain a provisional disclaimer -
Code.json --- Consider adding tags for mission area and center/region. In past I’ve been suggested to use this format.
"usgs_ma:Water", "usgs_sc:Science Center Name Here", -
Code.json --- Ensure metadataLastUpdated is current
Software Release Documentation
-
Python versions in readme img.shields currently do not match python versions referenced in the package README.md and meta.yaml files. Package details suggest 3.9 as minimum, but 3.8 is also listed in shields. -
With different versions of NHD being available I think it is important to specifically reference the version of NHD that being used (e.g., NHDPlusV2.1.) -
Some specific suggestions were submitted in the following pull request -
README.md Line 26: It appears rasterio is installed with the NLDI-Flowtools package. Do you still need “Rasterio must be installed prior to installing NLDI-Flowtools”? Is this included to help get at GDAL requirements, if so communicate that to the user. -
Ensure Docs and README specify geographic limitations that are defined in utils lines 83-89 (Only works in CONUS) -
Line 145 in utils.py, I’m assuming this can fail with 200 when you have a lat lon but the location is not in CONUS? If so express this in the error. -
Lines 261-263 in utils.py. Verify this error message. I do not believe this error message is logical given lat and lon are not being passed into this function (get_total_basin). -
Merge_geometry in utils could use more documentation. What is happening with the cofactor and variable d and what is the point of buffers. Also I’m guessing simplify is generalizing the geometry? -
Documentation used in the nldi_flowtools module is very helpful to me. I believe there is generally enough documentation in other modules but would suggest in future iterations to consider building out the parameter and return for other core functions/classes throughout the package. -
The package has a few functions that request lat, lon from a user but no CRS is suggested. Add the required CRS to the documentation throughout the package, or add CRS as a variable. -
Be consistent on use of term pour point. In the nldi_flowtools module pour point is for the most part consistently used (except for return raindropath definition mentions input point, but in readme the term pour point is never used.
Edited by Hopkins, Anders L