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

Refactor with method hierarchy #78

Closed
wants to merge 1 commit into from
Closed

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Feb 27, 2024

This is a draft PR that implements an alternative workflow (building on that discussed in #75) which uses multiple dispatch to allow use of both a struct and a function that takes arguments.

Implementation:

https://github.com/CDCgov/Rt-without-renewal/blob/refactor-struct-to-arg/EpiAware/src/latent-processes.jl

See example use:

priors = default_rw_priors()
n = 10  # Number of steps in the random walk

# Directly using the tag-based dispatch
latent_rw = latent_process(RandomWalkLatentProcessArg(), n, var_prior = truncated(Normal(0.0, 0.05), 0.0, Inf),
	init_prior = Normal())

# Create a RandomWalkLatentProcess instance with priors
rw_lp = RandomWalkLatentProcess(priors[:init_rw_value_prior], priors[:var_RW_prior])

# Using the struct-based dispatch
latent_rw_struct = latent_process(rw_lp, n)

# Attempting to call with a generic AbstractLatentProcess
latent_process(AbstractLatentProcess(), n)

# Attempting to call with a generic AbstractLatentProcessArg
latent_process(AbstractLatentProcessArg(), n)

Con

  • Adds an additional method that needs to be defined
  • May not be a standard approach
  • Leads to a slightly more complex stack trace
  • Has no code impact for our use case

Plus

  • Can document the arguments in the funciton docs and then link out to the struct that carries them
  • May make resuse of low level functions easier
  • Increases code modularity (?)

@@ -1,24 +1,41 @@
abstract type AbstractLatentProcess end
abstract type AbstractLatentProcessArg end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think defining a type hierarchy for the underlying priors and an auxiliary type hierarchy that is initimately connected to it is overkill.

For example, you can dispatch on the type of the first type without using it if you want to.

)
end

@model function latent_process(lp::RandomWalkLatentProcessArg, n;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels like the point of the above is to lower down to this function; but you can get the same effect by specifying the fields of RandomWalkLatentProcess (i.e. not using a NamedTuple or Dict of priors of potentially any name) and dispatching on that.

I'd avoid that by having RandomWalkLatentProcess have determined typed fields and saving a bunch of code repetition.

@SamuelBrand1
Copy link
Collaborator

SamuelBrand1 commented Feb 27, 2024

Its a pretty standard pattern to pass structs and dispatch on type information in Julia Packages; which I think you are trying hard to avoid here?

For example, in DifferentialEquations.jl a workflow might be:

  1. create a DEProblem struct (with different behaviours being ODEProblem, SDEProblem etc.
  2. Choose a solution method struct, e.g. Tsit5
  3. pass the above to solve e.g. solve(prob; Tsit5()) along with any other key words.

For example, scraping an example from here

function lorenz!(du, u, p, t)
    du[1] = 10.0 * (u[2] - u[1])
    du[2] = u[1] * (28.0 - u[3]) - u[2]
    du[3] = u[1] * u[2] - (8 / 3) * u[3]
    nothing
end

u0 = [1.0; 0.0; 0.0]
tspan = (0.0, 100.0)
prob = ODEProblem(lorenz!, u0, tspan)
solve(prob, Tsit5());

EDIT: even lowering right down to the time stepping we find functions with struct arguments e.g. here.

Base automatically changed from refactor-for-more-multi-dispatch to main February 27, 2024 21:25
@seabbs seabbs closed this in #79 Feb 27, 2024
@seabbs seabbs deleted the refactor-struct-to-arg branch February 27, 2024 21:26
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.

2 participants