Open jrising opened 1 year ago
Patch coverage: 69.73
% and project coverage change: -0.10
:warning:
Comparison is base (
61abb6b
) 84.52% compared to head (74874bd
) 84.43%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Here's the MimiFAIRv2 PR: https://github.com/FrankErrickson/MimiFAIRv2.jl/pull/16
@jrising this seems reasonable to me, am I right that in the diff most of the changes are the line spacing removal and the main functional change is just the addition of the method?
@davidanthoff does this look alright to you?
I'll note that we can also handle this by using the built-in Monte Carlo functionality of Mimi that takes care of the updating of a model instance without building, which is a bit safer for more novice users, but @jrising I know the FAIRv2 one we wrote did not use that functionality and as of now I haven't added one (I have for a paper I'm working on though, so when I get the chance I can port over that option too) so this is likely useful for your workflows. Happy to approve.
@lrennels I only checked in files with changes, so there are minor additions to each of those files. Here's the list of actual additions:
src/core/build.jl function build(mm::MarginalModel) function Base.run(mi::MarginalInstance; ntimesteps::Int=typemax(Int))
src/core/connections.jl function update_param!(mi::MarginalInstance, comp::Symbol, param::Symbol, value) function update_param!(mi::MarginalInstance, param::Symbol, value)
src/core/model.jl function dim_count(mi::MarginalInstance, dim::Symbol)
src/core/types/model.jl struct MarginalInstance function Base.getindex(mi::MarginalInstance, comp_path::Symbol, name::Symbol)
I tried to put these where the equivalent code is for MarginalModels or ModelInstances. There are probably other functions one might want for MarginalInstance to support, but these were the ones I found necessary.
Sorry for the delay @jrising I'll look at this this week, I think all should be fine I just want to run our dependency tests as well. I will note that I think I won't advertise this use for now, only because ew don't have as many safetys put onto theInstance
structures so they would be dangerous to use if you aren't more of an expert (as of course you are :) ).
For example I was reminded the hard way recently that if you run update_param!
on a ModelInstance
and the size of the array you update is not the same as the one you are updating ... it uses copyto!
so you won't get an error that warns you that you are only replacing part of the array. Makes for some super confusing behavior :)
No worries. Thanks for thinking about it.
Currently, one can only run
build!
on aMarginalModel
, notbuild
. The second is useful for Monte Carlo runs because it gives aModelInstance
where parameters can be updated without requiring new building. This extends that feature toMarginalModel
s.A
MarginalInstance
is a pair ofModelInstance
s, with the delta so that marginal values can be retrieved. It also has a (currently minimal) set of associated functions so that Monte Carlo code that currently takes aModel
can just be generalized toUnion{Model, MarginalModel}
and the associated code still works.In the diffs below, sorry that my editor removed spaces at the end of lines. If it's a problem, I can fix that.
An associated PR to MimiFAIRv2 that uses this change will come next.