Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax orthogonal #53

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Relax orthogonal #53

merged 3 commits into from
Jul 28, 2022

Conversation

AlexRobson
Copy link
Contributor

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
Copy link

codecov bot commented Jul 6, 2022

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
Copy link
Contributor Author

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.

@@ -46,7 +46,7 @@ be a positive-definite matrix (see https://en.wikipedia.org/wiki/Definite_matrix

The unconstrained parameter is a `LowerTriangular` matrix, stored as a vector.
"""
function positive_definite(X::StridedMatrix{<:Real})
function positive_definite(X::AbstractMatrix{<:Real})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation was orthogonal but I included this too

@rofinn
Copy link
Contributor

rofinn commented Jul 6, 2022

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.

@rofinn rofinn requested review from rofinn and willtebbutt July 6, 2022 21:45
@AlexRobson AlexRobson merged commit 3ff6a53 into master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants