Closed TimTaylor closed 1 year ago
@thibautjombart - Let me know what you think. It is a very large refactor. You will need the development version of grates (main branch). Best to build the pkgdown site locally to get a feel for things. Main outstanding task for me is to think about #88 and then get on to fixing the dependencies (mainly i2extras) for compatibility.
Thanks for this, I will get to it this week.
Note: I have created a project to store a backlog of tweaks to make once the version2 has been merged, mostly things I have noticed while reviewing the PR which are not deal-breakers and should not hold off the merging, but could be useful to do before the release.
On the essential points that are not currently in dedicated issues.
bringing back the interval argument in incidence; it provides a major convenience for generating different epicurves quickly, and would be simpler than having users resort to as_period (even if under the hood, this is what happens).
Assuming we go forward with version 2 this is a very deliberate design choice. The problem is that unless we force the choice of the date grouping on to the user it is not transparent to them what class the resulting grouped dates are. This works well when there is little flexibility in date groupings and when the classes are part of the package but not with the more flexible design of version 2.
bringing back the facet_plot: I like the current implementation better, but for compatibility purposes we could have this function as a wrapper around plot with a deprecation message
I'm ok for you to add a deprecated function for facet_plots(). The previous ones were extremely messy and hacky so may not port over easily to the new design though.
plots: discuss how different date indices and groups are handled, including:
- removing default color when there is but on date index and no group
- avoiding the default to dodge for x positioning as it creates an illusion of delay
- defaulting to facets for different dates indices as the y-scale could be widely different (e.g. reported cases vs deaths)
- bringing back the default y-axis labels
Unsure on 3. With version 2, the intention was to push plotting choices on to user as handling all of the different facet possibilites gets very hacky and could perhaps be dealt with better by users just treating the output as a data frame. Happy for you to experiment here.
the use of na_as_zero: it seems to me we should only ever get NAs in counts for a given time period / group from pre-aggregated counts clearly indicating NA in some entries; if so, these NAs should be kept as is; any other NAs occuring from completing combinations should be zeros (same rationale as in complete_counts); maybe we could remove the argument?
I think what you say about na_as_zero
makes sense. I just need to check there wasn't another reason for this behaviour but if not will make the change.
na_as_zero
parameter now removed via https://github.com/reconverse/incidence2/pull/91/commits/9fd7c82f0b9a038f1f07c786fa97a7fec5bb6736 . It actually had less scope then intended after the refactor anyway so sensible to remove.
I've now implemented more appropriate default plots:
Closing this. I'm going to squash and then push to main. @thibautjombart - I've addressed most of what we've discussed and we utilise issues to finalise prior to release.
This commit is a major refactor of incidence2 that we are undertaking for the 2.0.0 release. Whilst we are making many changes internally, the two big changes that will affect users are (1), the removal of support for non-standard evaluation (NSE); and (2), the removal of the date manipulation functionality.
Our motivation for removing support for NSE are both the complexity it can bring to the underlying code (making long term maintenance harder) and the complexity it can cause for other users / developers who want to build on top of incidence2.