Adds MarginalInstance struct

[jrising]

Currently, one can only run build! on a MarginalModel, not build. The second is useful for Monte Carlo runs because it gives a ModelInstance where parameters can be updated without requiring new building. This extends that feature to MarginalModels.

A MarginalInstance is a pair of ModelInstances, 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 a Model can just be generalized to Union{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.

[codecov]

Codecov Report

Patch coverage: 69.73% and project coverage change: -0.10 ⚠️

Comparison is base (61abb6b) 84.52% compared to head (74874bd) 84.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
- Coverage   84.52%   84.43%   -0.10%     
==========================================
  Files          40       40              
  Lines        3878     4074     +196     
==========================================
+ Hits         3278     3440     +162     
- Misses        600      634      +34     

Flag	Coverage Δ
unittests	84.43% <69.73%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/core/model.jl	83.33% <0.00%> (-3.63%)	⬇️
src/core/types/model.jl	65.21% <0.00%> (-22.29%)	⬇️
src/core/build.jl	97.47% <44.44%> (-2.53%)	⬇️
src/core/connections.jl	89.20% <83.05%> (-0.60%)	⬇️

... and 26 files with indirect coverage changes

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

[jrising]

Here's the MimiFAIRv2 PR: anthofflab/MimiFAIRv2.jl#16

[lrennels]

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

[jrising]

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

[lrennels]

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 :)

[jrising]

No worries. Thanks for thinking about it.