Fractions Skill Score post-merge followups

nci / scores

Metrics for the verification, evaluation and optimisation of forecasts, predictions or models.

https://scores.readthedocs.io/

Apache License 2.0

48 stars 15 forks source link

Fractions Skill Score post-merge followups #501

Open tennlee opened 3 months ago

tennlee commented 3 months ago

[ ] Add mathjax to the API docstring for FSS 2D
[ ] Add mathjax to the API docstring for FSS 2D Binary
[ ] Add mathjax to the API docstring for FSS 2D Single Field
[x] Fully test dask OR write a new issue for what needs to be done with enough detail another developer could do it if they wanted to
[ ] FSS 2D Binary docstring needs parameters, returns, references
[ ] In FSS 2D Single Field, don't use all-caps for the author names
[ ] Check rendering of all docstrings (see https://scores.readthedocs.io/en/stable/maintainer.html#common-rendering-issues-in-readthedocs)
[x] For the tutorial, please check against https://scores.readthedocs.io/en/stable/maintainer.html#tutorial-rendering
[x] For the tutorial, also follow the binder link and check it looks okay

nikeethr commented 3 months ago

re: dask testing - will have to be a separate issue. It can sometimes conflict with optimizations used by numpy, although the "parallel" option should in theory be compatible. see: https://github.com/nci/scores/issues/505

re: "In FSS 2D Single Field, don't use all-caps for the author names": will do this - just noting that this was the default output of the doi-based auto citation generator.

nikeethr commented 3 months ago

re: FSS 2D Binary docstring needs parameters, returns, references scores.spatial.fss_2d_binary:

@tennlee The entire function, args, references, description, typehints etc. is a verbatim copy (and essentially a wrapper, with the threshold omitted) of fss_2d. It already has a seealso banner to refer to the argument details from it's parent function. In my opinion, it adds too much clutter to repeat, but if you really want the duplication I can add it in.

nikeethr commented 3 months ago

docstrings have some random rendering issues that need fixing. Also they seem a bit more repetitive for some args, in the new typehint style e.g. for compute_method

tennlee commented 2 months ago

re: dask testing - will have to be a separate issue. It can sometimes conflict with optimizations used by numpy, although the "parallel" option should in theory be compatible. see: #505

That's fine. Please raise an issue for that (edit - sorry I overlooked that you already have). I have various tests in other modules which detect the importability of dask and then modify the test as appropriate. Let me know if you want a walkthrough of the hows and whys of the existing test setup.

re: "In FSS 2D Single Field, don't use all-caps for the author names": will do this - just noting that this was the default output of the doi-based auto citation generator.

Understood. No need to use the default output, it's okay to go ahead without all-caps.

tennlee commented 2 months ago

re: FSS 2D Binary docstring needs parameters, returns, references scores.spatial.fss_2d_binary:

@tennlee The entire function, args, references, description, typehints etc. is a verbatim copy (and essentially a wrapper, with the threshold omitted) of fss_2d. It already has a seealso banner to refer to the argument details from it's parent function. In my opinion, it adds too much clutter to repeat, but if you really want the duplication I can add it in.

If you think it adds too much clutter, just leave it be, but please take a final look and just consider it. The see-also should help.

tennlee commented 2 months ago

docstrings have some random rendering issues that need fixing. Also they seem a bit more repetitive for some args, in the new typehint style e.g. for compute_method

Do you need anything from me regarding this, or do you intend to go ahead and fix these yourself? Just let me know if I need to contribute or answer a question.