Change function specfication for approx#17
Conversation
hellemo
left a comment
There was a problem hiding this comment.
Looks like a nice improvement already!
I guess this will be a breaking change. So maybe also discuss other API changes, e.g. allowing to pass multi-dimentional arrays as an alternative to the FunctionEvaluations. (I can open an issue to follow up this if you agree).
There was a problem hiding this comment.
It looks good.
As we are supporting N-dimensional approximations, I think a multi-dimensional array is more general than multiple inputs.
Perhaps a sequence could be to work on the API changes (multidimensional vector) in a separate issue and then improve documentation before merging this PR?
|
Agree with all above. We should have a push for a new version with API changes and improved doc. |
|
I have added a few extra alternatives for input to the |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
|
I think a simpler interface like this can be a nice improvement, as defining the My main concern with the matrix input is that it is ambiguous what the dimensions represent. I think the suggested format makes sense since julia is column-major, but many users will probably have data in one observation per row format from tables in databases or spreadsheets, and will need to do a transpose first. Can we perhaps check the input dimensions and/or add to the documentation that you need to transpose in these cases (maybe even include an example)? |
|
Here is a suggestion with an automatic transpose if there are more rows than columns (with a warning). There should be more and better documentation if this is to be included. Currently, there are just some minor examples in the readme. |
|
I actually think the default matrix format should be the other way around, now it is row major as far as I can tell, and we need to transpose from the natural format before passing to using PiecewiseAffineApprox, JuMP, HiGHS
optimizer = optimizer_with_attributes(HiGHS.Optimizer, MOI.Silent()=>true)
xg = [i for i ∈ -1:0.5:1]
yg = [j for j ∈ -1:0.5:1]
X = [repeat(xg, inner = [size(yg, 1)]) repeat(yg, outer = [size(xg, 1)])]
z = X[:, 1] .^ 2 + X[:, 2] .^ 2
# Create Matrix to pass to approx
m = hcat(X, z)
count(x->true, eachcol(m))
# output
3
np = 17
pwa = approx(
m,
Convex(),
MILP(
;optimizer,
planes = np,
strict = :outer,
metric = :l1,
),
)
# output
# ┌ Warning: Transposing input matrix assuming row-based format
# └ @ PiecewiseAffineApprox ~/code/PiecewiseAffineApprox.jl/src/convexapprox.jl:36
# PWAFunc{Convex, 2} with 17 planes:
# z ≥ 0.5 x₁ + -1.5 x₂ + -0.5
# z ≥ 1.5 x₁ + -1.5 x₂ + -1.0
# z ≥ -15.75 x₁ + 0.5 x₂ + -14.75
# z ≥ -0.5 x₁ + 0.5 x₂ + -0.0
# z ≥ -1.5000000000000004 x₁ + 0.5000000000000004 x₂ + -0.5000000000000004
# ⋮
# z ≥ 1.5 x₁ + 1.4999999999999967 x₂ + -0.9999999999999967
# z ≥ 0.5 x₁ + -0.5 x₂ + -0.0
# z ≥ 1.5 x₁ + 0.5 x₂ + -0.5
# z ≥ -1.5 x₁ + -1.5 x₂ + -1.0
# z ≥ 0.5 x₁ + -1.5 x₂ + -0.5# output
|
This PR addresses some of the difficulties reported in issues #15 and #16. My suggestion is to not have the function to be approximated have a tuple as an input, but be general functions with multiple inputs.
Documentation is not yet aligned with the proposed changes, so some more work is needed before potentially merging.