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

Feature: Redesign interface #112

Merged
merged 64 commits into from
Jul 18, 2022
Merged

Feature: Redesign interface #112

merged 64 commits into from
Jul 18, 2022

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Jul 11, 2022

This PR aims to resolve #57. See the issue for interface design details and comments below for other changes.

@seabbs
Copy link
Collaborator Author

seabbs commented Jul 11, 2022

Currently, something I am a bit stuck on is the interface for priors. At the moment this is centralised and it would be nice if it wasn't. I think for now I am going to propose sticking with the current system as I don't really have a good idea but this does mean a future release will need to also have breaking changes. Once the dust has settled a bit if there is still no good solution will open an issue.

@seabbs
Copy link
Collaborator Author

seabbs commented Jul 13, 2022

This now implements everything discussed in #57 (excluding the bells and whistles in the expectation model which would anyway be non-functional) but I have yet to add documentation for the new functions, testing, or do anything about the failing CRAN checks so it isn't quite ready for review.

That being said as it is feature complete it would be good to get feedback at this stage. In particular on the setup of the modularity. This is all in R/model-modules.R and is linked together in R/epinowcast.R. At the moment everything is implemented with a simple list structure but potentially it would be useful to build out a s3 class etc but I think this can be done later/during another dev pass without breaking changes.

Something that is currently less than ideal is the passing of priors, in particular, you cannot set priors for fixed effects. This makes things a bit clunky. In other work (https://github.com/seabbs/epict/blob/main/R/model.R and https://github.com/seabbs/epict/blob/a462b2482d2ddd176085e12ed824b48c42eb1a26/R/extract.R#L59) I've done a bit more on this and also got linking with model output working. I am still not happy with that implementation so planning on punting this to a later PR (which may sadly add some breaking changes but hopefully to less used functionality).

Note: I have implemented interfaces for the expectation and missingness models which should make implementing them easier. The blockers for those features are updating the likelihood in the case of the missingness model (#64) and deciding what to do about the order of the expectation process (i.e is it on log cases, growth rate, or support multiple (with this being where I am landing and something that requires some small amount of thought to future proof against the addition of renewals etc + how to handle intercepts being allowed/not (as the order 2 model needs an initial intercept whereas the order 1 model doesn't))

@seabbs seabbs changed the base branch from develop to bug-max-delay-clumping July 16, 2022 19:56
Base automatically changed from bug-max-delay-clumping to develop July 16, 2022 20:03
@epinowcast epinowcast deleted a comment from codecov bot Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #112 (57b76f8) into develop (56d99db) will increase coverage by 20.80%.
The diff coverage is 97.05%.

@@             Coverage Diff              @@
##           develop     #112       +/-   ##
============================================
+ Coverage    64.29%   85.10%   +20.80%     
============================================
  Files           12       12               
  Lines         1042     1188      +146     
============================================
+ Hits           670     1011      +341     
+ Misses         372      177      -195     
Impacted Files Coverage Δ
R/utils.R 65.71% <ø> (ø)
R/epinowcast.R 90.00% <89.47%> (+90.00%) ⬆️
R/model-tools.R 93.75% <91.54%> (+55.37%) ⬆️
R/model-modules.R 99.24% <99.24%> (ø)
R/check.R 85.00% <100.00%> (+3.75%) ⬆️
R/formula-tools.R 96.48% <100.00%> (+4.33%) ⬆️
R/model-design-tools.R 91.80% <100.00%> (+4.91%) ⬆️
R/model-validation.R 95.00% <100.00%> (+0.55%) ⬆️
R/preprocess.R 95.60% <100.00%> (-0.04%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec963b6...57b76f8. Read the comment docs.

@seabbs seabbs mentioned this pull request Jul 18, 2022
Copy link
Collaborator Author

@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.

LGTM apart from linting flagged test gaps (self-review)

@seabbs seabbs merged commit 9ad47bb into develop Jul 18, 2022
@seabbs seabbs deleted the feature-redesign-interface branch July 18, 2022 15:10
@seabbs seabbs mentioned this pull request Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redesign interface
1 participant