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

Add a CFR module #20

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

pratikunterwegs
Copy link

Hi @PaulC91 and @ntncmch, I'm opening this PR following our discussions around adding a module to calculate CFR using the {cfr} package. I've implemented a basic module for now to make sure this is in line with the overall goals of {epishiny}, and listed some possible next steps below. Also cc-ing @adamkucharski and @sbfnk for input.

  1. This PR adds the CFR module as paired UI and server functions, cfr_ui() and cfr_server(). The server function takes only a single dataframe of aggregated daily case and death data as input. Groups are currently not supported.
  2. The module prints a single plot with the estimate over time, with 95% confidence intervals.
  3. The module allows for either a rolling CFR (i.e., with an expanding time series) which is suitable for understanding how additional data affects the estimate, or a time-varying CFR which is suitable for longer outbreaks where the severity is expected to change due to e.g. changes in case definition, variants, vaccines, etc.
  4. The module allows for delay correction in CFR estimates using the delay_density functionality in cfr; currently, normal, gamma, and log-normal distributions are supported with appropriate parameters.
  5. This module adds the {cfr} and {stats} packages as dependencies.

Potential next steps

  1. Adding a test suite for the module output,
  2. Adding an annotation with the overall CFR (the static CFR),
  3. Adding support for data grouping,
  4. Integrating the module with the overall dashboard,
  5. Adjusting graphical elements,
  6. Adding support for further distributions.

Example module output from the vignette:
Screenshot 2023-12-06 at 15 37 13

@PaulC91
Copy link
Member

PaulC91 commented Dec 8, 2023

Hi @pratikunterwegs.

Nice job on this! It's looking like a great interface to the cfr package. I have a couple suggestions that I think would improve the UX:

Use bindEvent to avoid unwanted client/server interaction

Currently code is being re-run and the chart re-rendered every time an input changes. Since there are quite a few input options I would imagine the typical user would commonly be changing 2 or more inputs before reaching the config they want. This would trigger code to be re-run multiple times unnecessarily before the user gets the output they are waiting for.

You can solve this by adding a shiny::actionButton at the bottom of the options dropdown and telling the cfr_estimate reactive to only invalidate when this button is clicked via shiny::bindEvent(). I would also add another bindEvent to the cfr_plot_hc reactive telling it to only invalidate when cfr_estimate() changes.

This way cfr estimates will only be run when the user has made all their input choices and clicked the button, and the plot will only update when these estimates are ready.

Update highchart with highchartProxy

This is less important but definitely a nice feature if you can make it work. Proxies allow you to update elements of an existing html widget (in this case a highchart) without re-rendering the whole thing from scratch. Again, this reduces the amount of data that needs to be sent between the server and the client and also allows for cool animations of chart elements when they are updated. For example, if you updated the data values for the 'arearange' and 'line' series in the cfr plot, you would see the range and line shift to their new values on the chart rather than the whole chart going blank and then being re-built.

See this script in the highcharter repo for examples of how to use the various proxy functions.

Integration into dashboard

I've been thinking about ways to integrate this into an epishiny dashboard but due to the data format requirements of cfr the options are quite limited. Currently it would only work if the data used in the dashboard was aggregated daily data with date, cases and deaths columns, but epishiny has been designed to accept both aggregated and non-aggregated (linelist) data and does not enforce specific variable names.

So I think one improvement would be to allow the user to pass the columns names for these 3 variables via date_var, cases_var and deaths_var arguments so that they don't have to do the renaming themselves if their data doesn't already match the name requirements.

It would be nice to be able to use the cfr method within the time module epicurve when show_ratio_line in enabled but again this would only be appropriate under quite specific circumstances:

  • data is aggregated with daily cases and deaths
  • the user specifically requests a cfr estimate (currently any sort of ratio can be requested from the data)
  • the date interval aggregation input is set as daily (default is weekly)

It would be good to know if the cfr method could be applied to weekly or monthly data. We have some dashboards at epicentre that use a weekly aggregated cases and deaths data source and it would be nice to be able to use this package and module for more robust cfr estimations.

Let me know your thoughts!

Paul

@pratikunterwegs
Copy link
Author

Hi @PaulC91 thanks for taking a look - will turn these into issues on our fork for now and address them in this PR. All of this sounds good and I'll try and get it done next week.

So I think one improvement would be to allow the user to pass the columns names for these 3 variables via date_var, cases_var and deaths_var arguments so that they don't have to do the renaming themselves if their data doesn't already match the name requirements.

Regarding the aggregated vs linelist data, {cfr} does provide a prepare_data.incidence2() method, but that would require taking on {incidence2} as a dependency to convert from a linelist, and potentially multiple filtering steps as well. I'll try and implement the user-selection of variable columns from aggregated data as that's an easier first step.

It would be good to know if the cfr method could be applied to weekly or monthly data. We have some dashboards at epicentre that use a weekly aggregated cases and deaths data source and it would be nice to be able to use this package and module for more robust cfr estimations.

This is a request we've had previously and we're still trying to figure out the best way to implement this. It would be great if you could add your use case/support to epiverse-trace/cfr#76 so that it gets prioritised.

@pratikunterwegs
Copy link
Author

Thanks for the feedback @PaulC91 - I've added a couple of changes.

  1. The module server accepts a "date_var", "cases_var", and "deaths_var" column as suggested, with default values. I'm still unsure whether this allows integration into the dashboard.
  2. I've wrapped the cfr_estimate and cfr_plot reactives in a bindEvent() triggered by an action button as suggested, but this now prevents the plot from displaying. I'm not sure where this is hitting a snag as I can get event binding to work in smaller examples. It would be great if you could help diagnose the issue.

Just FYI I will be away on leave from the end of the week until 18th Jan 2024, so there may be some delay before this PR is merged.

@PaulC91
Copy link
Member

PaulC91 commented Dec 11, 2023

Thanks @pratikunterwegs. I'll look into the plot display issue. Enjoy your holidays!

@PaulC91
Copy link
Member

PaulC91 commented Dec 12, 2023

Hi @pratikunterwegs. Can you give me permissions to push to this PR so I can add some fixes? Details of how to do that here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Thanks!

@pratikunterwegs
Copy link
Author

pratikunterwegs commented Dec 12, 2023

Thanks @PaulC91, I'll tag @adamkucharski who should be able to do that (as I don't have the permissions). Otherwise, I can also make the changes if you leave edit suggestions in the review.

@adamkucharski
Copy link

I've added as collaborator on epiverse-trace/epishiny @PaulC91 – hope that's what you need (and future proofs for any other work that needs pulling across), but let me know if not.

@PaulC91
Copy link
Member

PaulC91 commented Dec 12, 2023

Thanks a lot @adamkucharski, it's working now. @pratikunterwegs I just pushed the changes fixing the bindEvent trigger to your fork. Let me know if all is working as expected on your end.

@pratikunterwegs
Copy link
Author

I've just pushed a small commit for line-length formatting.

@pratikunterwegs
Copy link
Author

Hi @PaulC91 - just getting back to this after being on leave for a while - I've rebased the PR on main as it was out of date. If there are any other blockers to merging this, feel free to tag me!

@pratikunterwegs
Copy link
Author

Just checking in to see if there's anything I can help with to get this PR merged?

@PaulC91
Copy link
Member

PaulC91 commented Mar 20, 2024

Hi @pratikunterwegs. Apologies for my slow reply, I haven't had anytime to work on epishiny this year yet.

One feature you mentioned that I don't think has been implemented yet is "Adding an annotation with the overall CFR (the static CFR)". This could be added to the highcharter::hc_title or highcharter::hc_subtitle of the plot.

I was also thinking, since this module is quite distinct from the others due to the specific required data format, it will most likely be used in isolation, so perhaps it would make more sense to include it directly in the cfr package itself?

I think it would be a nice feature to allow users to launch a shiny interface to the package directly, without having to do this from another package. I'd be happy to help port this over if you think this is a good idea. Otherwise, still very happy to include it here if that's preferable for you.

Let me know your thoughts.

Thanks,
Paul

@pratikunterwegs
Copy link
Author

Thanks @PaulC91 - thanks for pointing out the missing annotation - I can get that added this week.

I do appreciate that this is different from other modules, but we're trying to keep all Epiverse packages as light as possible, so I'm not sure we'd want to add a shiny app to {cfr} directly. We are however working on a couple of more pipeline focused projects where a CFR module might be a good fit. @adamkucharski - what would you prefer?

@pratikunterwegs
Copy link
Author

Hi @PaulC91 - sorry about the delay, but I've finally got the static estimate with CI added to the plot title, with the subtitle showing whether delay correction has been applied or not. Hope this works for you, happy to make edits if needed.

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

Successfully merging this pull request may close these issues.

3 participants