allow more Param types

[rafaqz]

trafficstars

@briochemc this PR addes select keywords everywhere so that we can filter by other wrapper types - e.g. AbstractParam. (and ignore for the case there are types in your object that don't reconstruct, but that's a minor detail)

@bgroenks96 you might want to have a look at this and give some feedback. We will need to changing the grouping as well to not always use Param.

But, from the new tests, this is the kind of this it adds with select:

struct FreeParam{T,P<:NamedTuple} <: AbstractParam{T}
    parent::P
end
FreeParam(nt::NT) where {NT<:NamedTuple} = begin
    ModelParameters._checkhasval(nt)
    FreeParam{typeof(nt.val),NT}(nt)
end
FreeParam(val; kwargs...) = FreeParam((; val=val, kwargs...))
FreeParam(; kwargs...) = FreeParam((; kwargs...))

sf = S2(
    FreeParam(99),
    FreeParam(7),
    Param(100.0; bounds=(50.0, 150.0))
)
params(sf; select=FreeParam) == (FreeParam(99), FreeParam(7))
m = Model(sf)
m[:val, select=Param] == (100.0,)
getindex(m, :val; select=FreeParam) == (99, 7)
m[:bounds, select=FreeParam] = ((1, 100), (1, 10)) 
m[:bounds] == ((1, 100), (1, 10), (50.0, 150.0))

Probably we want a @param MyParam macro to define all those constructors automatically

[codecov-commenter]

Codecov Report

Merging #39 (285168e) into master (f2890ca) will increase coverage by 1.12%. The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   79.88%   81.00%   +1.12%     
==========================================
  Files           4        4              
  Lines         174      179       +5     
==========================================
+ Hits          139      145       +6     
+ Misses         35       34       -1     

Impacted Files	Coverage Δ
src/ModelParameters.jl	100.00% <ø> (ø)
src/tables.jl	77.77% <33.33%> (ø)
src/model.jl	78.40% <78.00%> (+0.62%)	:arrow_up:
src/param.jl	88.63% <100.00%> (+1.79%)	:arrow_up:

---

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 f2890ca...285168e. Read the comment docs.

[bgroenks96]

Could I get some context on this? Why is this necessary?

It seems to me that filtering is already possible by table operations on Model, and so-called "fixed" parameters can simply be left unchanged on reconstruction. Making the Param type polymorphic seems like an unnecessary complication of the type semantics, and appears to me to conflict a bit with the original design, i.e. allowing user-customization by defining arbitrary additional fields on the Param instances.

[rafaqz]

The original design always had AbstractParam ?

But I get your point, there are 2 ways of achieving this - table/vector manipulation outside of the object, or modification of the param type.

This way is going to be better for type stability where that's required, and is pretty easy to specify by the user. But its also less flexible.

[bgroenks96]

I know the original design had AbstractParam, but I didn't think it really did anything? It seemed everything more or less assumed Param to be the type, either implicitly or explicitly.

I'm not sure this is better for type stability. The main type stability issue in using update is with the parameters being supplied as a Vector, or some indeterminate length collection, which makes it tricky (or sometimes impossible?) to make reconstruction fully type stable.

So, for example, if one works only with Param tuples, then manipulation via map/filter should also be entirely type stable.

In other words, I don't think this is the main problem hindering type stability... or am I missing something?

[bgroenks96]

I have changed my mind about this, but I think the changes here will conflict a lot with #44, so it might be better for me to simply copy these changes into that PR by hand rather than try to resolve the merge conflicts.

[rafaqz]

Nice to see you finally came around ;)

Probably making a separate PR afterwards is better, or do you need this for your PR?

[bgroenks96]

We can do it in another PR.