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

Mass rename of step to time in implementation #375

Merged
merged 4 commits into from Nov 8, 2022
Merged

Mass rename of step to time in implementation #375

merged 4 commits into from Nov 8, 2022

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Nov 4, 2022

No new features, but a fairly painful rename here.

For mode models we need to work in terms of "time", but for dust models we used "step". Seemed like a good idea at the time. However, this causes all sorts of pain in getting a uniform interface for both model types, and there is more of a concept of time in a discrete time model than there is a concept of step in an ode model so dust should be the one to change.

I've added a little explanatory section in dust_generator.R (via the template)

##' For discrete time models, dust has an internal "time", which was
##' called `step` in version `0.11.x` and below.  This must always
##' be non-negative (i.e., zero or more) and always increases in
##' unit increments.  Typically a model will remap this internal
##' time onto a more meaningful time in model space, e.g. by applying
##' the transform `model_time = offset + time * dt`; with this approach
##' you can start at any real valued time and scale the unit increments
##' to control the model dynamics.

Before this can be merged I need to prepare PRs to adapt dependent packages:

No changes required to:

There's another change to make at some point where we replace "step" with "time" within odin discrete time models to complete this, but that's an even more disruptive change that might be hard to make

This is ready for review, but probably best to consider all at once - particularly the odin.dust and mcstate ones

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 99.83% // Head: 99.83% // No change to project coverage 👍

Coverage data is based on head (f6ee988) compared to base (2c1ff23).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files          67       67           
  Lines        3542     3542           
=======================================
  Hits         3536     3536           
  Misses          6        6           
Impacted Files Coverage Δ
R/examples.R 100.00% <ø> (ø)
R/interface.R 100.00% <ø> (ø)
R/compile.R 100.00% <100.00%> (ø)
R/data.R 100.00% <100.00%> (ø)
inst/include/dust/dust_cpu.hpp 100.00% <100.00%> (ø)
inst/include/dust/filter.hpp 100.00% <100.00%> (ø)
inst/include/dust/filter_state.hpp 100.00% <100.00%> (ø)
inst/include/dust/gpu/dust_gpu.hpp 99.62% <100.00%> (ø)
inst/include/dust/gpu/filter.hpp 100.00% <100.00%> (ø)
inst/include/dust/gpu/filter_state.hpp 100.00% <100.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@richfitz richfitz marked this pull request as ready for review November 7, 2022 10:57
@richfitz richfitz merged commit a1bb3c7 into master Nov 8, 2022
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.

None yet

2 participants