This is a long list, but they are all minor suggestions -- looking good!
I will add a second issue with comments from section 5 later on. I'm going to take a break from reviewing for now and didn't want to lose these.
FWIW I did not have any issues rendering the document using quarto preview, but the suggested rmarkdown command in the readme produced a not-that-well-formatted html document with weird-looking output.
[x] Text on page 4: "Per hubverse convention, each column serves one of three purposes: denote which model has produced the prediction (called the “model ID”), provide details about what is being predicted (called the “task IDs”), or specify how the prediction is represented (called the “model output representation”)"
I don't think think the value column fits neatly into any of these three groups. The closest fit is the last, but the value is the prediction, not a specification of how the prediction is represented. How about if we make this into four groups? Alternatively, update the description of that group so that it also captures the predicted value.
[x] "For example, projections of incident COVID-19 cases in the US at different locations, amounts of time in the future, and under different assumed conditions may use a target column of “cum death”,"
The target example here is "inc case" -- update "cum death" in this sentence.
[x] "although the sample output type is not yet directly supported by the hubEnsembles package, and need to be converted to another output type first"
"and need to be" --> "and must be" or "and needs to be" or "and should be", also consider "first" --> "before computing the ensemble"
[x] Table 3 description of output_type_id for cdf: "Numeric within the support of the outcome variable: a possible value of the target variable"
We don't require that the prediction assign non-zero probability to each x, so it is not required that the output_type_id be in the support of a forecast distribution. How about if we just remove that phrase: "Numeric: a possible value of the target variable"
[x] Table 3 description of output_type_id for sample: "String specifying sample index"
The schema we've put together allows the hub administrators to specify whether they want this to be a string or an integer. Suggest updating to include both options: "Integer or string specifying the sample index"
[x] Table 4 pmf descriptions include the phrase "individual model bin probabilities at each target variable value"
although binned values will be a common (maybe the most common) use case for pmf forecasts, there may be some instances where the pmf gives a distribution over a variable that is "naturally categorical" or discrete without binning. Suggest removing the word "bin" here.
This is a particularly weak suggestion though, I note you use bin again below and I couldn't think of anything else to put in there. So maybe just keeping bin and using it consistently throughout, as you do now, is best? Or, what about "bin or category"? I'm ok with anything you decide on here.
[x] Table 4 caption: "... (as a quantile function, 𝐹−1(𝜃), cumulative distribution function, 𝐹(𝑥), or probability mass function 𝑓(𝑥))..."
noting that we aren't actually returning the full functions, but the values of those functions. I thought of two possible rephrasings:
"... (as a quantile F^-1(\theta), a cumulative probablity F(x), or a probability f(x))"
"... (as the value of a quantile function F^-1(\theta), a cumulative distribution function F(x), or a probability mass function f(x))..."
[x] Page 8: "The linear_pool() function implements the linear opinion pool (POL)"
"POL" --> "LOP"
[x] Table 5 caption: "only the median, 50% prediction intervals, and 90% prediction intervals are displayed"
Suggest "only the median and quantiles corresponding to the endpoints of central 50% and 90% prediction intervals are displayed". Open to simplifications here, the main comment is that the things shown are quantiles, not (directly) intervals
[x] Page 11: we introduce the example data from FluSight up here in section 4.1, but don't discuss the fact that they are based on FluSight submissions (I think FluSight is first mentioned in section 5, and it's implied but not stated that the two sets of forecasts in sections 4 and 5 are drawn from the same source). I think it'd be good to at least mention that the forecasts come from FluSight at this point. I could imagine saying that briefly and then noting that a more complete description will be given below, or just moving the relevant paragraph from section 5 up here.
[x] Page 12 plotting code (and other code snippets below): the alignment looks a little funny because the first line has 2 extra characters' worth of stuff in the prompt. If you update to use 4 spaces for indentation instead of 2, it looks a little better (though I am not sure if this would result in issues with long line lengths anywhere):
[x] Page 13: "These forecasts are made for the same task ID variables as the quantile forecasts of incident hospitalizations."
Noting that the target is different -- could amend this to, e.g., "These forecasts are made for the same task ID variables as the quantile forecasts of incident hospitalizations, other than the target, which is "wk flu hosp rate category" for these categorical predictions."
[x] Page 14: 'We also use model_id = "simple-ensemble-mean to...' --> add closing " after simple-ensemble-mean
[ ] Page 16: In my rendered pdf, text is wrapping off the bottom of the page. I don't know what we might do about this. :(
[x] Figure 5 colors: When I run this code, the color assigned to the linear pool vs the quantile mean ensemble swap for panel A and for panel B. In panel A, the linear pool is red, while in panel B, the linear pool is blue. This is weird! But we should standardize this and in particular we should ensure that the legend describes the colors used in panel B. Right now, according to the caption description it would appear that the linear pool has lower variance than the quantile ensemble!
This is a long list, but they are all minor suggestions -- looking good!
I will add a second issue with comments from section 5 later on. I'm going to take a break from reviewing for now and didn't want to lose these.
FWIW I did not have any issues rendering the document using
quarto preview
, but the suggestedrmarkdown
command in the readme produced a not-that-well-formatted html document with weird-looking output.[x] Text on page 4: "Per hubverse convention, each column serves one of three purposes: denote which model has produced the prediction (called the “model ID”), provide details about what is being predicted (called the “task IDs”), or specify how the prediction is represented (called the “model output representation”)"
value
column fits neatly into any of these three groups. The closest fit is the last, but the value is the prediction, not a specification of how the prediction is represented. How about if we make this into four groups? Alternatively, update the description of that group so that it also captures the predicted value.[x] "For example, projections of incident COVID-19 cases in the US at different locations, amounts of time in the future, and under different assumed conditions may use a target column of “cum death”,"
[x] "although the sample output type is not yet directly supported by the hubEnsembles package, and need to be converted to another output type first"
[x] Table 3 description of output_type_id for cdf: "Numeric within the support of the outcome variable: a possible value of the target variable"
[x] Table 3 description of output_type_id for sample: "String specifying sample index"
[x] Table 4 pmf descriptions include the phrase "individual model bin probabilities at each target variable value"
[x] Table 4 caption: "... (as a quantile function, 𝐹−1(𝜃), cumulative distribution function, 𝐹(𝑥), or probability mass function 𝑓(𝑥))..."
[x] Page 8: "The linear_pool() function implements the linear opinion pool (POL)"
[x] Table 5 caption: "only the median, 50% prediction intervals, and 90% prediction intervals are displayed"
[x] Page 11: we introduce the example data from FluSight up here in section 4.1, but don't discuss the fact that they are based on FluSight submissions (I think FluSight is first mentioned in section 5, and it's implied but not stated that the two sets of forecasts in sections 4 and 5 are drawn from the same source). I think it'd be good to at least mention that the forecasts come from FluSight at this point. I could imagine saying that briefly and then noting that a more complete description will be given below, or just moving the relevant paragraph from section 5 up here.
[x] Page 12 plotting code (and other code snippets below): the alignment looks a little funny because the first line has 2 extra characters' worth of stuff in the prompt. If you update to use 4 spaces for indentation instead of 2, it looks a little better (though I am not sure if this would result in issues with long line lengths anywhere):
yields this output:![image](https://github.com/Infectious-Disease-Modeling-Hubs/hubEnsemblesManuscript/assets/5132208/1d5f5265-a470-4346-b8bd-e39b9921c233)
[x] Figure 2 caption: "One example quantile forecast" --> pluralize "forecasts"
[x] Page 13: "These forecasts are made for the same task ID variables as the quantile forecasts of incident hospitalizations."
target
, which is "wk flu hosp rate category" for these categorical predictions."[x] Page 14: 'We also use model_id = "simple-ensemble-mean to...' --> add closing " after simple-ensemble-mean
[ ] Page 16: In my rendered pdf, text is wrapping off the bottom of the page. I don't know what we might do about this. :(![image](https://github.com/Infectious-Disease-Modeling-Hubs/hubEnsemblesManuscript/assets/5132208/6eae6bab-0ffa-43ea-a8d5-f89eead8ae4e)
[x] Figure 5 colors: When I run this code, the color assigned to the linear pool vs the quantile mean ensemble swap for panel A and for panel B. In panel A, the linear pool is red, while in panel B, the linear pool is blue. This is weird! But we should standardize this and in particular we should ensure that the legend describes the colors used in panel B. Right now, according to the caption description it would appear that the linear pool has lower variance than the quantile ensemble!