Software review
Software review
Thanks @dwieferich for the opportunity to review hydrolink. I think this will be a super helpful package. I think you've done a great job thinking about all the potential issues that may arise when trying to address locations particularly in complex regions of multiple nearby segments. I only have minor comments and for the most part they should be considered suggestions.
- I've run the tests and example notebooks with no issues or errors. I've run additional tests using my own coordinates in complex area's and the feedback from the code is great.
- I created a merge request with some suggested changes to the readme regarding conda environements. I use them most of the time regardless of what OS I'm working with, especially for geospatial workflows, because they greatly simplify the issues with GDAL.
- I checked the code to make sure the calculations of distance were done in an equal area projection - they are done in 5070 - which is appropriate.
- Check for bare excepts rather than except use
except Exception: There are a number of these in both nhd_hr.py and nhd_mr.py. - The code could be generally improved by using pre-commit: here is a good place to start Pre-commit can be set up to ensure your code follows PEP standards. For example, your often exceeds pep standards for line length. Generally setting up pre-commit with black, flake8, prettier and some others maybe of interest, will help your code base.
hydrolink.hydrolink_method
- if I add a non-sensical method like "blah" or slight misspelling "closet" there is no error trapping, and the downstream calls don't provide an error message that would help the user determine what the proper source of the area is.
- outfile_name parameter. I'm not sure what the solution is here, but at first it was confusing when I re-ran the notebooks and there are multiple answers provided when reading back the resulting csv file, which to a new user is hidden as a default parameter.
CLI
- I think it would suggest simplifying the CLI to just import a table with all the option fields and the coordinates. If you decide to keep the cli as is, then I would add an additional and shorter prefix to make using the cli easier :). For example (--input-file, -i, ...)
Future considerations
- you might also consider addressing against Dave Blodgett's mainstems network?
- This could easily be built into a web service.
Let me know if you have any questions or want to follow up on anything in the review.