Update docstring and docs for graphflood

[Teschl]

Changelog:

• graphflood.py
  • Converted import of C++ functions to new style I have been using to minimize the use of stuff like: type:ignore and such.
  • reordered imports to comply with pep8
  • Added a docstring to run_graphflood()
    • there is still a description missing for SFD.
    • I wasn't sure what the best description for the return would be.
  • Added parameter specifications to run_graphflood()
• docs/api.rst
  • added graphflood to docs, it's separate from the GridObject since it's not part of the class. Should we change that or keep it this way?

Argument names like N_iterations or SFD don't conform to snake_case naming style. Do we want to replace them or just keep them like this? I think it doesn't really do any harm to keep them this way, but if we want to be strict about pep8 we should adjust them.

[wkearn]

Thanks for cleaning this up, @Teschl! I probably want to get @bgailleton to look over this. It seems like mostly cosmetic changes, but I do want to make sure that things like the type specifications or the renaming don't interfere with his workflow.

Changelog:

* graphflood.py
  
  * Converted import of C++ functions to new style I have been using to minimize the use of stuff like: `type:ignore` and such.
  * reordered imports to comply with pep8

This is the kind of thing that we would like to catch in linting if we're going to enforce it. Is there a way to configure flake8 to alert us about the import order?

  * Added a docstring to `run_graphflood()`
    
    * there is still a description missing for SFD.
    * I wasn't sure what the best description for the return would be.

I added some comments with possibilities for those descriptions.

  * Added parameter specifications to `run_graphflood()`

* docs/api.rst
  
  * added graphflood to docs, it's separate from the GridObject since it's not part of the class. Should we change that or keep it this way?

For now I think it is all right if it is separate from the class.

We probably should develop some guidelines for which functions should be methods on our classes and which can be plain functions.

Argument names like N_iterations or SFD don't conform to snake_case naming style. Do we want to replace them or just keep them like this? I think it doesn't really do any harm to keep them this way, but if we want to be strict about pep8 we should adjust them.

I'll defer to @bgailleton for the moment regarding naming conventions for graphflood.

In the longer term, I think it will be important to have consistency within pytopotoolbox for named arguments: if we have an SFD argument to run_graphflood and also some future flow routing method on GridObject, we should make sure that they are spelled the same just so users aren't tripped up by needing SFD for one and sfd for the other. Sticking to the PEP8 snake case style is probably the easiest way to make sure this works, but we would probably also need some developer documentation that spells out things like "use sfd rather than single_flow_direction".

If we do want to take the naming conventions more seriously this is also something that we should catch in our linter.

[Teschl]

This is the kind of thing that we would like to catch in linting if we're going to enforce it. Is there a way to configure flake8 to alert us about the import order?

Pylint would catch issues like that. It's still in the ci.yaml pipeline but as a comment. While pylint would catch issues like wrong-import-order. But pylint also raises issues like:
src/topotoolbox/grid_object.py:16:0: R0902: Too many instance attributes (10/7) (too-many-instance-attributes)
These can be disabled with a .pylintrc file. So I will add this, but we'll have to decide which errors to ignore, then we can switch from flake8 to pylint (or use both).

Edit:
Or for the too-many-instance-attributes example, we could also include # pylint: disable=too-many-instance-attributes when needed.

[Teschl]

I enabled pylint in the ci.yaml file and added a few exceptions in the pyproject.toml. For example, "too-many-arguments" or "too-many-statements" (too much if / elif / else). I only disabled refactor suggestions which help to find code smells, but left convention linting (pep8) as is. I also modified the contributing section to better explain what tests we run.

The pipeline is failing as expected because: Argument name "[...]" doesn't conform to snake_case naming style.

[bgailleton]

Hi @wkearn @Teschl , sorry for the late reply. I'll try to have a look today or tomorrow!

[bgailleton]

OK I reviewed the modifications, and everything looks good to me!

For now, I suggest we focus on properly exposing/documenting the run_graphflood function. The other functions are needed internally to graphflood and necessary to keep, but they serve as specific utilities that are, or will be, available in topotoolbox in a better shape. For example, functions like computing topological order for single and multiple flow directions or filling depressions.

I originally created them to handle boundary conditions specific to the Shallow Water Equation solver I developed, which might make them less convenient or fully integrated with the rest of the libtopotoolbox.

Let me know what you think!

[wkearn]

@Teschl: I think we can go ahead and merge these changes and continue to think about our style conventions unless there is anything specific you think we need to address now. It looks like there is a failing lint at the moment, but if you sort that out and mark this PR as ready for review, I will merge it.

[Teschl]

@wkearn: I converted all remaining variable name to lowercase. It now passes the pylint tests. I also noticed that the project had no requirements.txt, so I added one. This resolved an issue with non-reproducible errors when using pylint (a .venv can now be created easily).

This should be ready to merge now.

[wkearn]

Excellent. I do want to get @bgailleton's thoughts about the argument case because it might have the potential to break things on his end.

We have been sticking more or less to PEP8 as a reasonable standard, but the point of a style guide is to ensure the project presents a consistent interface to users, not just to obey the linter. I am okay with breaking the style guide if it makes it easier to interface with existing code and as long as we can clearly specify what we expect to break with the style guide.

To that end, is it possible to turn off a particular pylint warning for a single function or class? That might be a reasonable solution for now, so we can continue to add stuff under the PEP8 lints while letting @bgailleton make decisions about the GraphFlood interface. And as he mentioned above, I think at some point we will probably want to unify the graphflood and traditional flow routing interfaces, which would be a good time to revisit some of these questions.

On a separate note: Why do we need requirements.txt? Is it just for ensuring that we have consistent versions of dependencies when pylint runs on GitHub Actions? Can we specify versions of things in pyproject.toml? I am a little nervous about having multiple places where we specify dependencies since it is easy for things to get out of sync.

[bgailleton]

I do not have strong opinion on argument case, especially if it makes things more consistent on your side. I can adapt my codes already relying on topotoolbox to match that requirement if needed.

In my opinion there are pros and cons to any standards anyway, enforcing lowercase enhance clarity for the developers, having capital letters can be more intuitive for non experimented users. In the end consistency is key and as I use topotoolbox as a backend I can hide the change in standards. Thanks for letting me know though, so I'll adapt to the change next release!

[Teschl]

To that end, is it possible to turn off a particular pylint warning for a single function or class?

Yes, by adding #noqa or # pylint: disable=name-of-warning depending on the linter, we can disable particular warnings in a file/line of code.

On a separate note: Why do we need requirements.txt? Is it just for ensuring that we have consistent versions of dependencies when pylint runs on GitHub Actions?

Yes, the requirements.txt is needed to enforce versions for pylint, mypy and pytest. But it's also used so developers can create a venv with the correct versions to be able to accurately reproduce what's happening in the .github/workflows. It also contains so many more dependencies because it also lists every dependency of a dependency.

Can we specify versions of things in pyproject.toml?

We can, I will add them after this.