Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additional comments on draft circulated to collaborators on May 6 #66

Open
12 tasks done
nickreich opened this issue May 6, 2024 · 2 comments
Open
12 tasks done

Comments

@nickreich
Copy link
Contributor

nickreich commented May 6, 2024

The paper looks great! I have a series of small comments, none of which I really see as essential or should hold up submission, but these are more minor details that I think would make it all just a bit tighter.

  • Table 1: (a) consider making the team name simpler. as it is now, it is distracting (hard for an initiated reader to understand what "MOBS-GLEAM_FLUH" is) and it requires the table to wrap to 2 lines per row. How about just "teamX-modelY" or something like that? or at least explain what that team/model abbreviation is. (b) can it be printed to not wrap onto a second page? (c) consider sorting by horizon, so the values for each modeling task are in order.
  • in general, is the model_id column ever introduced or discussed? if not, maybe adding one sentence about what its use it would be valuable.
  • There is a phrase now in the intro that reads: "Complementing other software for combining predictions from multiple models (e.g., (Pedregosa et al. 2011; Weiss et al. 2019; Bosse et al. 2023; Couch and Kuhn 2023)), hubEnsembles supports..." which has a second set of parentheses that should be removed if possible.
  • Table 2: same comment as above with model_id. either explain what the abbreviation is or make it easier to understand.
  • "The output_type_id provides additional identifying information for a prediction and is specific to the particular output_type (see Table 1)." should be "see Table 3".
  • Figure 1: somewhere it should be mentioned that the y-axis is log scale. maybe on the axis label itself and in the legend?
  • I'd suggest referencing Figure 1 when first introducing the mathematical notation for the ensemble in section 2, as the visual depiction of the calculations may be helpful for many readers not familiar with these ideas. The figure seems best where it is since it builds on definitions in section 3, but maybe a mention like "For a visual depiction of these equations, see Figure 1 below."
  • section 4 says "The figures from here on are generated re- producibly using data from rds files stored in the analysis/data/raw-data directory and scripts in the inst directory." but then also "We will use an example hub provided by the hubverse to demonstrate the functionality of the hubEnsembles package". It's not clear to me which is being used.
  • Figure 5: it looks like two levels of prediction intervals are shown, but this is not described in the legend.
  • in at least one place, "Flusight" is written, instead of "FluSight" with the second capital letter. it should be the "FluSight".
  • arguments inside the simple_ensemble() call are not appropriately indented in section 5.1. The lines with weights, agg_fun and model_id should start a few spaces in. This is what they look like now in the manuscript
R> mean_ensemble <- flu_forecasts_hubverse |>
+  hubEnsembles::simple_ensemble(
+  weights = NULL,
+  agg_fun = "mean",
+  model_id = "mean-ensemble"
+)
  • Add citation to hubverse documentation to first sentence of second paragraph of the discussion
@eahowerton
Copy link
Contributor

I've addressed most of these issues. A few follow up notes:

  1. I couldn't find a way to force tables onto a single page in Quatro. Maybe you know @lshandross?
  2. We introduce the model_id column in the second paragraph of the hubverse terminology section: "Each model should have a unique identifier thatis stored in the model_id column." Happy to add a bit more detail if it's needed.
  3. Fig. 1 y-axis isn't technically on a log-scale. The ticks/horizontal lines highlight the quantiles that are imagined to be "submitted to a hub". I've noted this in the legend, but didn't see a clear way to include it in the axis label itself.
  4. I believe the noted sentence in Section 4 does not belong, I think maybe it landed there by accident. I'll leave it open for @lshandross to relocate and/or chime in.

@lshandross
Copy link
Collaborator

(1) I can manually force tables to be only on one page if we prefer this (I do agree it looks cleaner), but note that I have also not found a way to do this otherwise.

(4) I can take care of moving this to somewhere in section 5 since I agree it does belong there more than in section 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants