-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Add a CFR module #20
Conversation
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
|
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.
Regarding the aggregated vs linelist data, {cfr} does provide a
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. |
Thanks for the feedback @PaulC91 - I've added a couple of changes.
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. |
Thanks @pratikunterwegs. I'll look into the plot display issue. Enjoy your holidays! |
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! |
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. |
I've added as collaborator on |
Thanks a lot @adamkucharski, it's working now. @pratikunterwegs I just pushed the changes fixing the |
I've just pushed a small commit for line-length formatting. |
35200ae
to
e587e23
Compare
Hi @PaulC91 - just getting back to this after being on leave for a while - I've rebased the PR on |
Just checking in to see if there's anything I can help with to get this PR merged? |
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 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, |
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? |
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. |
Just to add that upon further exploration, the {cfr} package method is also valid for individual-level data, especially if we suspect that the delay to outcome is different or many recoveries are not reported (which is mathematically the equivalent of an infinite delay to recorded outcome). Here's the PR with vignette describing use of {cfr} functions with line list data: epiverse-trace/cfr#170 |
Thanks @adamkucharski. I'll look to get this integrated into the package when I have some time to work on it (hopefully soon!) |
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.
cfr_ui()
andcfr_server()
. The server function takes only a single dataframe of aggregated daily case and death data as input. Groups are currently not supported.delay_density
functionality in cfr; currently, normal, gamma, and log-normal distributions are supported with appropriate parameters.Potential next steps
Example module output from the vignette: