Plot projections and eval scores #115
Conversation
There was a problem hiding this comment.
Awesome work, @Fuhan-Yang! I apologize that I'm not well-versed in altair, so I cannot review the actual plotting with much detail. But I did find one spot that I think needs a change - the forecasts are saved in long, not wide, format now. Otherwise, there are just some questions peppered throughout.
| data = pl.scan_parquet(args.obs).collect() | ||
|
|
||
| # Drop all samples and just use mean estimate, for now | ||
| pred = pred.drop([col for col in pred.columns if "estimate_" in col]) |
There was a problem hiding this comment.
This line expects the forecasts in wide format, but they are now in long format. See Line ~592 in iup/models.py, which returns predictions from a model as SampleForecasts (with sample_id and estimate columns), and then see Line ~85 in scripts/forecast.py, which averages over samples to get the mean of the posterior predictive distribution at each time_end before saving the forecasts.
There was a problem hiding this comment.
I forget if this was being discussed elsewhere, but I have a strong preference for long format for these things
There was a problem hiding this comment.
Thanks for catching that! This line may adapt from previous version and didn't raise error because the condition didn't meet. We indeed switched to long format, and just average over for now. This line is deleted.
|
|
||
| charts = [] | ||
|
|
||
| forecast_starts = [date for date in pred["forecast_start"].unique()] |
There was a problem hiding this comment.
Should we check that the pred frame is already sorted by forecast_start to make sure that the plot panels come out in a sensible order? I am unfamiliar with altair plotting, so maybe panel ordering is done automatically - just wanted to check.
There was a problem hiding this comment.
altair won't mind if the rows in the input data frame aren't sorted in the order that the points will be left-to-right plotted
| obs: polars.Dataframe | ||
| The observed uptake data frame, indicating the cumulative vaccine uptake as of `time_end`. | ||
| pred: polars.Dataframe | ||
| The predicted daily uptake, differed by forecast date, must include columns `forecast_start` and `estimate`. |
There was a problem hiding this comment.
I would say, "The predicted cumulative uptake, ..." since the predictions taken here are cumulative, whereas daily average uptake is a separate that actually appears in some of the model fitting.
| pred: polars.Dataframe | ||
| The predicted daily uptake, differed by forecast date, must include columns `forecast_start` and `estimate`. | ||
| config: dict | ||
| config.yaml to define the number of columns in the graph, the saving path and the saving name. |
There was a problem hiding this comment.
The config doesn't actually have the path or name for saving, does it?
There was a problem hiding this comment.
See above; agree I would drop config
There was a problem hiding this comment.
No, this was my trial but it got deprecated. Thanks for catching that
| scores: polars.DataFrame | ||
| The evaluation scores data frame. | ||
| config: dict | ||
| config.yaml to define the saving path and the saving name. |
There was a problem hiding this comment.
Again, the config doesn't really have the path or name for saving, right? I thought those were in the makefile. The config has the full names of the evaluation metrics, though.
There was a problem hiding this comment.
See above; agree that I would drop config
There was a problem hiding this comment.
Fixed. Thanks for catching
There was a problem hiding this comment.
My big things are:
- Making sure that config is not passed to the postprocessing plot scripts
- Simplifying the plots using existing facet or grid systems
I also don't think it's crazy to have a single postprocessing.py that plots the forecasts and the scores. Because at some point we'll also want some csv (or whatever) reports of some of these values. Rather than have a different script for each of them, we could just stick them all together. There aren't that many outputs being made, that this shouldn't be too slow? (Unless it is slow to make some of these plots, in which case, we should keep them separate!)
There was a problem hiding this comment.
I would put all of these options right in the scripts themselves
Like, the config is most important for articulating what analyses the pipeline is going to run, so the model names, model parameters, score funs, etc. are all important
But "plot: True" is something that should really be in the Makefile, and how many columns in the subplot is something we can just leave in the script and tweak there as needed
There was a problem hiding this comment.
Ditto with the score function -> name dictionary
I'd rather just have this defined somewhere in the plotting script, because changing it shouldn't trigger a re-run of the upstream forecasts
There was a problem hiding this comment.
I'm comparing against the flowchart in #61
I think this PR introduces dependencies from config.yaml to proj_plot.py and score_plot.py, when we don't really need those dependencies. I'd like to be able to iterate on the postprocessing without triggering a re-run of the upstream stuff
There was a problem hiding this comment.
Actually, looking at Makefile, it looks like the config is not passed to these scripts
There was a problem hiding this comment.
But "plot: True" is something that should really be in the Makefile, and how many columns in the subplot is something we can just leave in the script and tweak there as needed
Thanks for pointing out what are important for config file, which I found vague so far. How to put "plot: True" in Makefile to let users choose to plot or not? I will put the number of subplot columns in the script, and would like you to demo the plotting option in Makefile!
|
|
||
| forecast_starts = [date for date in pred["forecast_start"].unique()] | ||
|
|
||
| for forecast_start in forecast_starts: |
There was a problem hiding this comment.
Why not just use a grid chart with .encode(row=etc, column=etc) or a facet chart with .facet()?
See https://altair-viz.github.io/user_guide/compound_charts.html#repeated-charts for explanations of how to avoid constructing these kinds of grids manually
(I'm not even sure you need layers here, to do data and projection; you might be able to just unpivot and use .encode(color=)
| scores: polars.DataFrame | ||
| The evaluation scores data frame. | ||
| config: dict | ||
| config.yaml to define the saving path and the saving name. |
There was a problem hiding this comment.
See above; agree that I would drop config
| pred: polars.Dataframe | ||
| The predicted daily uptake, differed by forecast date, must include columns `forecast_start` and `estimate`. | ||
| config: dict | ||
| config.yaml to define the number of columns in the graph, the saving path and the saving name. |
There was a problem hiding this comment.
See above; agree I would drop config
Add diagnostic plots of time-varying projections compared with data, and evaluation scores differed by forecast dates. Minor bugs showed up when using different forecast periods. Worth to try multiple forecast periods to ensure the robustness of the pipeline.