JuliaGaussianProcesses / ParameterHandling.jl

Foundational tooling for handling collections of parameters in models
MIT License
72 stars 11 forks source link

Fix type instability in unflattening Tuples #67

Closed simsurace closed 9 months ago

simsurace commented 9 months ago

On master this does not infer:

julia> tpl = (1., ((2., 3.), 4.));
julia> par, unflatten = flatten(tpl);
julia> @code_warntype unflatten(par)

This PR fixes this and (so I believe) improves the type inference with a recursive implementation and a fallback for empty tuples.

willtebbutt commented 9 months ago

Thanks for opening this. Busy morning -- will try to review later today

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (567b070) 97.36% compared to head (f5bdf67) 97.40%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #67 +/- ## ========================================== + Coverage 97.36% 97.40% +0.03% ========================================== Files 8 8 Lines 228 231 +3 ========================================== + Hits 222 225 +3 Misses 6 6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

willtebbutt commented 9 months ago

I think views definitely make sense here. Very open to that.

While you do that, I'm going to drop 1.5, because we really don't need it.

simsurace commented 9 months ago

Thinking it over a bit, I think going to views should be evaluated a bit more carefully, as it also requires generalizing a lot of the signatures of the unflatten functions. I will keep that for a later PR perhaps.

simsurace commented 9 months ago

I'm gonna suggest approving this as is. I bumped the version. Maybe we can release #68 in the same swoop?

willtebbutt commented 9 months ago

Thinking it over a bit, I think going to views should be evaluated a bit more carefully, as it also requires generalizing a lot of the signatures of the unflatten functions. I will keep that for a later PR perhaps.

Good point. Yeah, let's defer that until later. I do think it would be good to look at though, because we're currently doing a lot of copying.

simsurace commented 9 months ago

No problem!