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

Changing DP.Polyvar -> DP.Variable as suggested #829

Closed
wants to merge 3 commits into from

Conversation

stephanmg
Copy link
Contributor

@stephanmg stephanmg commented Nov 16, 2023

@lkapelevich here you go anyway but still some tests error here.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f81cf2) 96.38% compared to head (21086ce) 96.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #829      +/-   ##
==========================================
+ Coverage   96.38%   96.77%   +0.39%     
==========================================
  Files          56       56              
  Lines        8971     8971              
==========================================
+ Hits         8647     8682      +35     
+ Misses        324      289      -35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@lkapelevich lkapelevich left a comment

Choose a reason for hiding this comment

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

Thanks @stephanmg for the fix, and sorry I didn't think the suggestion through enough.

I found two more updates that are breaking the examples:

  1. In examples\convexityparameter\JuMP.jl, line 72 could be bss() = SAS.FullSpace() and on line 77 addinequality!->add_inequality!
  2. In examples\JuMP_utils.jl, MOI.Bridges.add_bridge(opt, B{Float64}) -> MOI.Bridges.add_bridge(opt, B)

I would prefer for someone else (not me) to make the fixes, if you are up for it.

@@ -105,7 +105,7 @@ function get_lagrange_polys(pts::Matrix{T}, deg::Int) where {T <: Real}
end

# returns the multivariate Chebyshev polynomials in x up to degree deg
function get_chebyshev_polys(x::Vector{DP.PolyVar{true}}, deg::Int)
function get_chebyshev_polys(x::Vector{DP.Variable{true}}, deg::Int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just have this as Vector{<:DP.Variable}, the DP types seem to have acquired even more parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also below on line 114 Vector(undef, PolyUtils.get_L(n, deg)) seems to work and it would be nice not to have to update these DP types in the future

Copy link
Member

@blegat blegat Nov 17, 2023

Choose a reason for hiding this comment

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

Yes, <:DP.Variable seems safer or even <:MultivariatePolynomials.AbstractVariable

@@ -124,7 +124,7 @@ function get_chebyshev_polys(x::Vector{DP.PolyVar{true}}, deg::Int)
return V
end

function get_chebyshev_univ(monovec::Vector{DP.PolyVar{true}}, deg::Int)
function get_chebyshev_univ(monovec::Vector{DP.Variable{true}}, deg::Int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto Vector{<:DP.Variable}, ditto Vector(undef, deg + 1) on line 134

H::Matrix{<:DP.Polynomial}
is_feas::Bool # whether model should be primal-dual feasible; only for testing
use_wsosmatrix::Bool # use wsosinterppossemideftri cone, else PSD formulation
use_dual::Bool # use dual formulation, else primal formulation
end

function SemidefinitePolyJuMP{Float64}(
x::Vector{DP.PolyVar{true}},
x::Vector{DP.Variable{true}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto thoughts on Vector{<:DP.Variable}

examples/contraction/JuMP.jl Show resolved Hide resolved
@chriscoey
Copy link
Collaborator

Thanks @stephanmg and @lkapelevich. tagging @blegat here in case he has any thoughts.

@stephanmg
Copy link
Contributor Author

Hi @chriscoey I tried to add most of @lkapelevich suggestions, however, please double check.

@chriscoey
Copy link
Collaborator

@stephanmg good changes! the polynomial examples are still failing though - https://github.com/chriscoey/Hypatia.jl/actions/runs/6900952711/job/18774937852?pr=829
and I think some suggestions from Lea and Benoit above will help fix those

@lkapelevich
Copy link
Collaborator

moved to #830

@stephanmg stephanmg deleted the fix_polyvar branch November 27, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants