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

156 solve like function which dispatches on epiawareproblem and epiawaremethod #169

Conversation

SamuelBrand1
Copy link
Collaborator

@SamuelBrand1 SamuelBrand1 commented Mar 25, 2024

A draft PR for an apply_method method.

Methods

The basic idea is that each behaviour of a method, which means either a sampling or optimization technique, has an _apply_method dispatch which implements this.

An important method is EpiMethod implies doing a sequence of optimizations passing infomation to each next level, then finally to a sampling method NUTSampler.

apply_method

apply_method has two methods:

  1. ingests a Turing model object and applies the epi method.
  2. Ingests an EpiProblem and a data object, as well parameters to fix and/or condition on, creates an appropriate model and then apply the epi method.

The output of apply_method is a NamedTuple which returns samples as well as gens, that is generated quantities if that makes sense for the returned object from _apply_method, and model which is the model (after any conditioning or fixing of parameters).

TBD

  • Naming?
  • Return objects?
  • More unit tests

Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think this all looks good. Can look through in more detail or happy to wait until it goes from draft to full.

@SamuelBrand1
Copy link
Collaborator Author

SamuelBrand1 commented Mar 26, 2024

Its failing the

Testing NegativeBinomialError against theoretical properties`

test on CI but not locally. Any thoughts?

@SamuelBrand1
Copy link
Collaborator Author

In general I think this all looks good. Can look through in more detail or happy to wait until it goes from draft to full.

I think a detailed review would be good.

@seabbs seabbs force-pushed the 156-solve-like-function-which-dispatches-on-epiawareproblem-and-epiawaremethod branch from 1eda853 to c4cce0d Compare March 28, 2024 11:04
@CDCgov CDCgov deleted a comment from codecov-commenter Apr 16, 2024
@seabbs
Copy link
Collaborator

seabbs commented Apr 16, 2024

@SamuelBrand1 this is ready for review I think.

There is an outstanding issue with the manypathfinder into NUTs in the getting started example that I think is unrelated but I am really not clear on.

We also need to open some new issues to improve areas of this but as long as we do that I think we should move to merge asap as this is now very large, blocking, and complicated (none of which is ideal). I've closed some of the testing gaps but there are still a few I'd suggest we port to issues. I also think there could be someone better solutions for quite a few of the changes here but again I think we should split those out and implement as independent PRs

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 68.67470% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 92.17%. Comparing base (6a6a1d4) to head (a99f900).

Files Patch % Lines
EpiAware/src/EpiAwareBase/apply_method.jl 0.00% 6 Missing ⚠️
EpiAware/src/EpiAwareBase/functions.jl 0.00% 6 Missing ⚠️
EpiAware/src/EpiAwareBase/EpiProblem.jl 55.55% 4 Missing ⚠️
EpiAware/src/EpiAwareBase/generate_models.jl 0.00% 3 Missing ⚠️
EpiAware/src/EpiAwareUtils/turing-methods.jl 86.36% 3 Missing ⚠️
EpiAware/src/EpiAwareBase/EpiAwareObservables.jl 33.33% 2 Missing ⚠️
EpiAware/src/EpiInference/ManyPathfinder.jl 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
- Coverage   99.31%   92.17%   -7.14%     
==========================================
  Files          33       37       +4     
  Lines         293      358      +65     
==========================================
+ Hits          291      330      +39     
- Misses          2       28      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SamuelBrand1
Copy link
Collaborator Author

OK. I'm looking at this and the getting_started example

Copy link
Collaborator Author

@SamuelBrand1 SamuelBrand1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, quick takeaway. I like the restructure in the files.

Copy link
Collaborator Author

@SamuelBrand1 SamuelBrand1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at getting_started now

@SamuelBrand1
Copy link
Collaborator Author

Want to make me a reviewer? I'm distant enough I think due to going on holiday in the middle of this PR?

@seabbs
Copy link
Collaborator

seabbs commented Apr 17, 2024

I can't because you opened it. We need to just mutually agree we are okay with merging and then can do

@SamuelBrand1
Copy link
Collaborator Author

I can't because you opened it. We need to just mutually agree we are okay with merging and then can do

I'm good. I approve.

@seabbs seabbs linked an issue Apr 17, 2024 that may be closed by this pull request
@seabbs seabbs merged commit ca38828 into main Apr 17, 2024
10 checks passed
@seabbs seabbs deleted the 156-solve-like-function-which-dispatches-on-epiawareproblem-and-epiawaremethod branch April 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants