Quantile Interval Score and Interval Score API Docstrings
Closed this issue · 11 comments
@reza-armuei @nicholasloveday @tennlee
I am currently taking a look at the API entries in readthedocs for Quantile Interval Score (https://scores.readthedocs.io/en/latest/api.html#scores.continuous.quantile_interval_score) and Interval Score (https://scores.readthedocs.io/en/latest/api.html#scores.continuous.interval_score).
I noticed a few minor issues. As I didn't manage to review the API entries while the pull request was still open, I am opening an issue instead.
The following issues relate to both API entries (for Quantile Interval Score and for Interval Score)
-
Both APIs list an incorrect DOI for Gneiting, T., & Raftery, A. E. (2007). I think the correct DOI is https://doi.org/10.1198/016214506000001437 (please do confirm that is the correct DOI).[Fixed: @tennlee has corrected the DOI in both API entries] -
A note about formatting for API entries in readthedocs:
- A single set of backticks will cause the enclosed text to be italicised. I.e. `quantile_interval_score` will be rendered as quantile_interval_score
- If you want the enclosed text to be rendered in a grey box (so to speak) - e.g.
quantile_interval_scoreorfcst_lower_qtile- you need to use two backticks on either side of the text (i.e. ``quantile_interval_score`` or ``fcst_lower_qtile``). - For both API entries, I am unclear in which instances you want the text to be italicised, and in which instances you wish for the text to be rendered in a grey box.
Please note: this specific backtick rendering only applies to API docstring rendering in readthedocs (it does not apply to non-API-docstring rendering in readthedocs, e.g. it does not apply to tutorial rendering in readthedocs). It took us a while to figure this out, and we still have docstrings we need to go and fix this in!
Some minor feedback about the Quantile Interval Score docstring in readthedocs (https://scores.readthedocs.io/en/latest/api.html#scores.continuous.quantile_interval_score).
Please note: I did not review the example, as I don't have the domain expertise to do so.
-
Typo in the first sentence: "Calculates the qauntile interval score for interval forecasts." Should be "quantile". -
Second sentence: "penalize". For the most part, thescoresdocumentation uses Australian/UK English spelling, rather than US English spelling (although I think there is the odd US English spelling in the docs). Australian/UK English spelling would be "penalise". -
Questions 5a-5d are regarding the following:
PLEASE NOTE: (A) I don't know how to format text as subscript in GitHub issue comments. If you use any of the following suggestions, please use subscript text as appropriate. (B) If any/all of my suggestions below are inaccurate or inappropriate, please ignore them.
5a. "qu is the forecast at upper quantile": should it be "at the upper quantile" (adding "the")?
5b. "ql is the forecast at lower quantile: should it be "at the lower quantile" (adding "the")?
5c. "αu is upper quantile level": should it be "is the upper quantile level" (adding "the")?
5d. "αl is lower quantile level": should it be "is the lower quantile level (adding "the")?
-
In parameters, it is inconsistent whether each dot point ends with a full stop or not. I don't think this matters very much, but I noticed it, so thought I would mention it. -
Re. the preserve dims dot point: "preserve_dims (Iterable[Hashable] | None) – Optionally specify which dimensions to preserve when calculating quantile interval score." Should this be "calculating the quantile interval score" (adding "the")? -
Re. the weights dotpoint: "weights (DataArray | None) – Optionally provide an array for weighted averaging (e.g. by area, by latitude, by population, custom)". Would it be worthwhile specifying that this weighting is different to threshold weighting? I honestly don't know if that would be useful or not, I am just raising the question. (Apologies if this was already discussed in the initial pull request). If you do decide this is worth specifying, I am not sure how to best phrase it.
-
Re. the returns section:
Is this how you want the formatting to appear? If so, no worries.
Re. the Interval Score docstring (https://scores.readthedocs.io/en/latest/api.html#scores.continuous.interval_score):
I haven't reviewed the Interval Score docstring in readthedocs properly as yet. As already mentioned, I wasn't sure when you wanted text in italics versus in a grey box (so to speak).
Aside from that, at a glance, it looks like some of the same/similar issues may be present as in the Quantile Interval Score docstring.
If I get a chance, I will come back and provide a review of the Interval Score docstring as rendered in readthedocs. Alternatively, if you update the Interval Score docstring first, I am happy to try and review that pull request.
Thank you. Regarding your first point, issue (1) of the incorrect DOIs. I have now corrected the DOIs on the develop branch. Thank you very much for noticing this
A single set of backticks will cause the enclosed text to be italicised. I.e.
quantile_interval_scorewill be rendered as quantile_interval_score
If you want the enclosed text to be rendered in a grey box (so to speak) - e.g. quantile_interval_score or fcst_lower_qtile - you need to use two backticks on either side of the text (i.e.quantile_interval_scoreorfcst_lower_qtile).
Alternatively, you can do :py:func:`quantile_interval_score` since it's a function.
Re. the weights dotpoint: "weights (DataArray | None) – Optionally provide an array for weighted averaging (e.g. by area, by latitude, by population, custom)". Would it be worthwhile specifying that this weighting is different to threshold weighting?
Potentially change it to something like "Optionally provide an array for weighted averaging (e.g. by area, by latitude) that may be needed when aggregating the mean score across dimensions. Alternatively it could be used for masking data."
I'm sure that there is a more elegant way of writing that.
Hi @Steph-Chong , Thank you so much for your review. All your comments are valid and I addressed them in #733.
I have conducted a review of #733 and put strikethroughs against the parts of this issue which appear to me to have been addressed.
Regarding (6) I think this has been done but I found it hard to pick out the relevant parts by eye so someone else might like to take a look.
Regarding (8) I'm comfortable with it as-is, but @nicholasloveday might have an opinion.
I have pointed out two minor grammar issues in the PR review.
I'm happy with Reza's update to (8) in the PR.
#733 has been merged. I will leave the issue open for a little longer so people can confirm everything is resolved, and if there is no further comment I will resolve this issue in a few days.
In the interval score docstring, in the example, the first sentence says: "Calculate the interval score for forecast intervals with interval range of 0.5 (i.e., lower and upper quantile levels are 0.25 and 0.75, respectively)." I think this should be "with an interval range of 0.5" (adding an "an").