Readthedocs/API

[barneydobson]

Description

This PR is not supposed to be the comprehensive complete documentation - just a starting point to which new things can be added.

• Introduce basic docs pages
• Added quickstart.py (I know it needs some love - it is more to have a starting point) see below
• You can build by pip install -r doc-requirements.txt and mkdocs serve (the initial one may take some time as quickstart takes a few mins)
• I know the way I've done API keys in the quickstart is dumb - addressed in API keys via env variables #94
• tqdm looks rough in the mknotebooks (similar to in WSIMOD ) - I had a go but I've got no clue how to fix it short of turning off verbosity, but I think the verbosity is useful so people can see the logs of what is going on. - no longer relevant for this - though will possibly have to return to it in other notebooks.
• I could add quickstart to tests... but it does involve data downloads, which as we've established is not ideal in a test. I could provide the download data, but then that isn't the best demonstration since the downloads won't run... not sure on this one.
• Not sure how to fix the coverage.md issue in the gaurav-nelson/github-action-markdown-link-check@v1 action - the file is generated after that CI action is run as part of the testing (I think... I admit I don't super follow every step of the CI)
• Small improvement to how to handle no real data provided

Update 2024-06-21
OK sorry this has added a bit more than anticipated, but hopefully you'll find it to be an improvement, set out below.

• So I have moved away from the interactive python notebook as the quickstart. I appreciate it probably added too much complexity for the initial starting point.
• Instead I have just gone for a simple markdown with CLI+config as @cheginit suggested (quickstart.md). Hopefully this is a bit more understandable.
• In doing this, I realised that we could provide default values for a lot of the things that can be provided in the config (specifically, graphfcn_list, metric_list, run_settings and precipitation). Thus, I use the 'default' values for these if they are not provided. By doing this, it really enables a super minimal but valid config file for a user to get started with.
• To accommodate this, and I know @cheginit we decided not to do this, I have added back in the automatic streamorder calculation inside derive_subbasins_streamorder. I am still in agreement with you that this is not ideal, but 1) it does flag a warning message, and 2) we are agreed that the delineation will one day need further improvement (ideally when we have access to a few more high quality urban drainage network data) - at which point this automatic streamorder selection can be removed.

Fixes #105 #210

[cheginit]

You don't need to use reshape, by default (with axis=None), unique returns the unique elements as a 1D array, regardless of the array shape.

[cheginit]

So I have moved away from the interactive python notebook as the quickstart. I appreciate it probably added too much complexity for the initial starting point. I have just gone for a simple markdown with CLI+config as @cheginit suggested (quickstart.md). Hopefully this is a bit more understandable.

It's fine at this stage since for the paper, we eventually have to have notebooks for reproducing the results.

doing this, I realised that we could provide default values for a lot of the things that can be provided in the config (specifically, graphfcn_list, metric_list, run_settings and precipitation). Thus, I use the 'default' values for these if they are not provided. By doing this, it really enables a super minimal but valid config file for a user to get started with.

I am always in favor of setting default values for parameters with secondary effects on the model output.

to accommodate this, and I know @cheginit we decided not to do this, I have added back in the automatic streamorder calculation inside derive_subbasins_streamorder. I am still in agreement with you that this is not ideal, but 1) it does flag a warning message, and 2) we are agreed that the delineation will one day need further improvement (ideally when we have access to a few more high quality urban drainage network data) - at which point this automatic streamorder selection can be removed.

Have you tested this automatic approach that was in the notebook that I shared?

idxs, _ = flw.snap(xy=(x, y))
subbasins = flw.basins(idxs=np.unique(idxs))

where x and y are network nodes. I remember discussing it, but I don't remember if you tested it or not.

[barneydobson]

@cheginit Hmm it does look like it would handle anything. If I understand it's returning basins that contain the most points, so that all points are contained. If I understood it properly it will tend towards the largest possible basins, for me then I think it makes sense as an alternative fallback instead of the while loop? Though I'd be cautious to use this instead of allowing user specified streamorder since they may want smaller basins

[cheginit]

@barneydobson It doesn't necessarily favor larger basins. The snap tool tries to find the immediate downstream. So, if a collection of points are located upstream of a large river segment, they get assigned the same subbasin. So, it depends on the river network.

If this approach works for the test cases that you have right now, then my suggestion is to use it as the default option and keep the stream order for cases where it fails.

[barneydobson]

@cheginit OK converted to your proposed method above - maybe I made it more cumbersome than necessary but it seems to pass the tests at least

[cheginit]

It appears that you implemented it the opposite way, i.e., stream order is the default and the snap when it fails? I meant something like this: By default the min stream order arg is None so the function uses the snap method only. If the min stream order arg is not None then the function ignores the snap method and uses the stream order only. This way, the user knows exactly which method is being used.

[barneydobson]

@cheginit Ahh I misread - yes this is a much better suggestion, allowing customisation if desired and using the default otherwise

[barneydobson]

@cheginit flipped them around now

[cheginit]

Yeah, this is much better!