Add `value_flatten` as described in #37.

[jonniedie]

I couldn't think of a better name than value_flatten. If you can think of one, feel free to change it or kick it back to me to change.

[codecov[bot]]

Codecov Report

Merging #38 (58a44c1) into master (d6c1045) will increase coverage by 0.65%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   95.83%   96.49%   +0.65%     
==========================================
  Files           4        4              
  Lines         168      171       +3     
==========================================
+ Hits          161      165       +4     
+ Misses          7        6       -1     

Impacted Files	Coverage Δ
src/ParameterHandling.jl	100.00% <ø> (ø)
src/flatten.jl	98.07% <100.00%> (+0.11%)	:arrow_up:
src/parameters.jl	97.43% <0.00%> (+1.28%)	: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 d6c1045...58a44c1. Read the comment docs.

[willtebbutt]

Thanks for opening this.

The main question I have is whether we need to formalise this as a separate interface, or just have a single value_flatten function that we lightly test, and indicate isn't the correct thing for people to extend? i.e. continue to say that they should just extend value and flatten, and assume that value_flatten is simply a thin layer of convenience functionality on top?

Regarding the name: seems fine. We can always deprecate in favour of something else later on if we think of something better.

[jonniedie]

Yeah, that makes sense. I'll change that.

[willtebbutt]

Great. This all makes sense to me now.

Could you update the examples in the README while you're here? Specifically, appending to the existing code in the Dealing with Constraints section something about "we also provide the utility function value_flatten which is equivalent to value(unflatte(v))" or something along those lines. Similarly, in the worked example section, I can't see any reason not just replace the current manual composition of value and unflatten with your new function.

[willtebbutt]

Thanks for adding these. Beyond the style issues, the only issue I have is for picking a convention for the output of value_flatten. For flatten I'd been using unflatten (for obvious reasons), but for value_unflatten I had been using unpack to differentiate it from unflatten.

Do you have any thoughts on what we should do here? Or do you think that there's unlikely to be confusion caused by having unflatten refer to both the output of flatten and value_flatten?

[jonniedie]

I have a slight preference for calling both unflatten, but I don't have any great reasons for it. I don't think it will be confusing because unflatten is already a generic name for a function that only has meaning in the specific context it was created. But something like value_unflatten would work too (although it's a little verbose).

I actually find unpack to be a slightly confusing, since it does the opposite of what the popular @unpack macro from UnPack.jl/Parameters.jl does (in their case, you @unpack from a structured object and @pack! back into one). I mean, in this case we're unpacking from a flat vector, so it's not like the term is being incorrectly used or anything, it's just a different direction w.r.t the data structure than I'm used to thinking of it. But maybe that's just me because I use @unpack so often with ComponentArrays models.