search

probabilistic-numerics / probnum

Probabilistic Numerics in Python.
http://probnum.org
MIT License
438 stars 57 forks source link

Remove TODOs or change them to correct format #69

Open JonathanWenger opened 4 years ago

nathanaelbosch commented 4 years ago

The linked PR is merged. Is there still anything actionable here? If yes it might be good to explicitly write what should be done, if not we should close this.

JonathanWenger commented 4 years ago

While the previously linked PR #59 is merged there are still tons of TODOs in mastere.g. in probnum.linalg.linearsolvers.matrixbased.

nathanaelbosch commented 4 years ago

The occurrence of TODOs can now be checked more easily, with 

pylint src --enable="fixme"

So the part about "removing todos" is well defined, as having this command pass without errors. Once done, the ./pyproject.toml should also be adjusted to include this message into future pylint runs.

@JonathanWenger Do you have a similar definition in mind for "removing comments"? I'm not sure that all comments should actually be removed, as there might be some valid cases. See eg. the google style guide (which also has a nice part about todo comments btw)

nathanaelbosch commented 4 years ago

Update: I added the "fixme" pylint exception only to specific modules. Resolving these is therefore also a part of #158, #159, #160.

JonathanWenger commented 4 years ago

There seem to be some legitimate situations that warrant TODO comments in the code in my opinion. For example for some functions of random variables there is in principle a way to compute them and even a paper reference, but no capacity to implement them at this point. I'd suggest we stick to the Google style guide wherever possible, so the actionable part of this issue is to add names to all remaining TODO comments in the google style: 

# TODO(kl@gmail.com): Use a "*" here for string repetition.
# TODO(Zeke) Change this to use relations.