add Const wrappers #67
Conversation
|
I forgot But |
I don't think it's a good idea to add dispatches to |
|
Nice work, good to see this functionality finally added. In general, it looks good. I somehow feel like it's maybe not ideal for |
| the `val` field, or a combination of `val` and `units` if a `units` field exists. | ||
| """ | ||
| function stripparams end | ||
| function strip end |
There was a problem hiding this comment.
Why not strip_params_and_consts or stripall?
There was a problem hiding this comment.
Yes maybe stripall? - it actually unwraps any Model too so just params_and_consts still doesn't quite capture what it does. Just that strip would be really nice syntax, unfortunate that verb was wasted on string whitespace.
Currently I use stripparams everywhere but the name will be misleading after this, so I want it to break next release so it has to be changed.
| allkeys = Tuple(union(map(keys, pars)...)) | ||
| _expandkeys1(pars, Val{allkeys}()) | ||
| end | ||
| Base.@assume_effects :foldable function _expandkeys1(pars, ::Val{Keys}) where Keys |
There was a problem hiding this comment.
does @assume_effects actually do anything here? also does this lower bound us to Julia 1.11? I would be against that.
There was a problem hiding this comment.
maybe it was only the annotations of code blocks that was added in 1.11... I don't remember, I haven't really used this macro.
There was a problem hiding this comment.
Its from 1.10. I think :foldable is so that map over Keys will always compile away.
|
|
||
| params(m::AbstractModel) = params(parent(m)) | ||
| stripparams(m::AbstractModel) = stripparams(parent(m)) | ||
| strip(m::AbstractModel) = strip(parent(m)) |
There was a problem hiding this comment.
As discussed in the main thread, I would suggest not doing this.
There was a problem hiding this comment.
Yeah, we just need a good verb to replace it
|
You may be right maybe we should make AbstractParam and AbstractConst part of the same heirarchy ? I'm not sure what we would call the supertype. |
|
I think it's hard to come up with a common base type because constants are actually parameters... they are just parameters that we don't want to change. I would also actually expect |
|
I honestly cannot think of a use case where I would want to do |
|
Yes absolutely. So |
|
Seriously considering this change, we can make |
Alternatively, But I'm OK with both approaches, I think. |
Closes #66
This PR adds
Constwrappers to easily move between constant/forward parameters and variable parameters.@bgroenks96 if you have some thoughts on this approach and if its useful that would help. Ive used all the param code in
@evalloops to ensureConstandParamare essentially identical.Needs tests