[WIP] Use scalar parameters with ParameterHandling#397
[WIP] Use scalar parameters with ParameterHandling#397
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
theogf
left a comment
theogf
left a comment
There was a problem hiding this comment.
Just did a first pass
Two general comments:
- By using
onlywe could get rid of a lot of thelength(v) = = 1 - In the composite kernels, instead of
vcateach of the resulting flattened vectors, why not useflattenagain to get the reconstruction for free
|
|
||
| # or just `@noparams GibbsKernel` - it would be safer since there is no | ||
| # default fallback for `flatten` | ||
| function ParameterHandling.flatten(::Type{T}, k::GibbsKernel) where {T<:Real} |
There was a problem hiding this comment.
Is flatten on Function defined? If yes I think it is fine to throw an error and to tell to implement the needed function for the used struct
|
|
||
| See also: [ParameterHandling.jl](https://github.com/invenia/ParameterHandling.jl) | ||
| """ | ||
| struct ParameterKernel{P,K} <: Kernel |
There was a problem hiding this comment.
I chose Parameter because I felt many kernels are parameterized and hence Paramet(e)rizedKernel might be confusing.
There was a problem hiding this comment.
Actually didn't we mention another more meaningful name like KernelLayer or something of this taste? Maybe KernelModel?
There was a problem hiding this comment.
Yes, I think in my initial sketch I used KernelLayer (probably since we discussed its use with Flux at this point) and in the more concrete suggestion I used KernelModel. I thought ParameterKernel was a bit better but I'm not sold on the name and change it to whatever we think is best.
I know, and initially I used it in some places 🙂 However, it requires Julia >= 1.4 and hence does not work with Julia 1.3. |
|
Since this is going to a be a (massively) breaking release, maybe we can drop support < 1.6 |
|
I would be fine with this but I didn't want to make this change without prior discussion. Also I assume Julia 1.6 might be the LTS before this PR is ready and merged 🙂 |
|
|
||
| See also: [ParameterHandling.jl](https://github.com/invenia/ParameterHandling.jl) | ||
| """ | ||
| struct ParameterKernel{P,K} <: Kernel |
There was a problem hiding this comment.
Yes, I think in my initial sketch I used KernelLayer (probably since we discussed its use with Flux at this point) and in the more concrete suggestion I used KernelModel. I thought ParameterKernel was a bit better but I'm not sold on the name and change it to whatever we think is best.
Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #397 +/- ##
==========================================
- Coverage 90.43% 0.49% -89.95%
==========================================
Files 52 54 +2
Lines 1390 1631 +241
==========================================
- Hits 1257 8 -1249
- Misses 133 1623 +1490 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
This PR implements #299 (comment). See the comment and the additional explanations in the issue for more details.
This PR is still WIP and tests will fail (I assume).