JuliaGaussianProcesses / ParameterHandling.jl

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

Relax orthogonal #53

Closed AlexRobson closed 2 years ago

AlexRobson commented 2 years ago

One issue with this I've found recently revisiting orthogonal is that it restricts wrapper arrays. As an example, present internal pipelines use AxisKeys and NamedDims - however the StridedArray restriction prevents this:

using ParameterHandling: value, flatten

A = NamedDimsArray{(:id, :_)}(rand(5,5));

orthogonal(A)
ERROR: MethodError: no method matching orthogonal(::NamedDimsArray{(:id, :_), Float64, 2, Matrix{Float64}})
Closest candidates are:
  orthogonal(::StridedMatrix{var"#s3"} where var"#s3"<:Real) at /Users/alexrobson/.julia/packages/ParameterHandling/AGJx1/src/parameters.jl:181

Rewriting orthgonal to allow for abstractmatrix allows this to work:

using NamedDims, ParameterHandling

A = NamedDimsArray{(:id, :_)}(rand(5,5));

v, unflatten = flatten(orthogonal(A));

unflatten(v) # ParameterHandling.Orthogonal{NamedDimsArray{(:id, :_), Float64, 2, Matrix{Float64}}

With a nod to #22 AFAICT only flatten needs to be specialised?

codecov[bot] commented 2 years ago

Codecov Report

Merging #53 (6a65c56) into master (0d59a83) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #53   +/-   ##
=======================================
  Coverage   96.80%   96.80%           
=======================================
  Files           7        7           
  Lines         188      188           
=======================================
  Hits          182      182           
  Misses          6        6           
Impacted Files Coverage Δ
src/parameters_matrix.jl 100.00% <100.00%> (ø)

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 0d59a83...6a65c56. Read the comment docs.

AlexRobson commented 2 years ago

I didn't include any test changes here as it's just a relaxation, would add a test-dep, and covered by existing. But the example code in the description can easily be converted into them.

rofinn commented 2 years ago

Yeah, based on what the functions are doing I don't see a strong reason to restrict the type. I'd appreciate @willtebbutt's input on this though given #22. Don't forget to update the docstrings.