frazane / scoringrules

Scoring rules for probabilistic forecast evaluation and optimization.
https://frazane.github.io/scoringrules/
Apache License 2.0
41 stars 6 forks source link

Extend CRPS documentation #1

Open frazane opened 1 year ago

frazane commented 1 year ago
nicholasloveday commented 3 months ago

Additionally, it would be good to list the available estimators in the documentation. I think that crps_ensemble can take several.

I had to go digging through the code to find the available options here https://github.com/frazane/scoringrules/blob/main/scoringrules/core/crps/_gufuncs.py#L346

nicholasloveday commented 3 months ago

In the documentation it has

>>> from scoringrules import crps
>>> crps.ensemble(pred, obs)

This raises an error. Instead, I had to do

import scoringrules
scoringrules.crps_ensemble(obs, pred)

Note that the forecast and observed args are also out of order when comparing the function implementation to the existing docs and need to be swapped.

nicholasloveday commented 3 months ago

With the typehints you have "ArrayLike" and "Array" as strings for the forecast and observed data types. This isn't particularly useful when looking at the docs to see what data types it can takes. E.g., https://github.com/frazane/scoringrules/blob/main/scoringrules/_crps.py#L11

Is there a reason why you don't use scoringrules.core.typing.Array and scoringrules.core.typing.ArrayLike in the type hints? I see that there is an if statement in the import so you may have a reason for it being this way and things may also get complicated with supporting multiple backends.

I had to go looking in https://github.com/frazane/scoringrules/blob/main/scoringrules/core/typing.py to see what data types it could handle. It would be nice if NDArray | JaxArray | torchTensor | tensorflowTensor etc appeared in the docs rather than "Array"

nicholasloveday commented 3 months ago

The example for twCRPS for ensembles doesn't use the v_func arg which is a required function. It would be good to give an example of setting up and using this chaining function.

frazane commented 2 months ago

With the typehints you have "ArrayLike" and "Array" as strings for the forecast and observed data types. This isn't particularly useful when looking at the docs to see what data types it can takes. E.g., https://github.com/frazane/scoringrules/blob/main/scoringrules/_crps.py#L11

Is there a reason why you don't use scoringrules.core.typing.Array and scoringrules.core.typing.ArrayLike in the type hints? I see that there is an if statement in the import so you may have a reason for it being this way and things may also get complicated with supporting multiple backends.

I had to go looking in https://github.com/frazane/scoringrules/blob/main/scoringrules/core/typing.py to see what data types it could handle. It would be nice if NDArray | JaxArray | torchTensor | tensorflowTensor etc appeared in the docs rather than "Array"

@nicholasloveday

Because of the multi-backend support, types are defined dynamically with typing.TypeVar, with the bound being the union of the supported array types. The reason they are not used directly is that the import statements related to types are not executed at runtime, but only by static type checkers (see https://peps.python.org/pep-0484/#runtime-or-type-checking), so as to not incur in ImportError if some backends are not installed. Maybe I should opt for try except statements during import?

When using modern IDEs like VSCode, these type hints defined as strings are handled like forward reference and are still "clickable" and give you the information you need. But it's true that it would be better if these also appeared in the documentation. I will definitely look into that. Thanks for the feedback!