Skip to content

Add MOI interface and expand docs, experiments and tests#95

Open
Vaibhavdixit02 wants to merge 14 commits into
mainfrom
paperrevisions
Open

Add MOI interface and expand docs, experiments and tests#95
Vaibhavdixit02 wants to merge 14 commits into
mainfrom
paperrevisions

Conversation

@Vaibhavdixit02

Copy link
Copy Markdown
Member

This pull request introduces major improvements to the documentation and extends the DCP atom registry in SymbolicAnalysis.jl to include explicit MathOptInterface (MOI) cone annotations for all standard atoms. It also adds new documentation files and connects the conic form and MOI bridge modules to the main package. These changes make it easier to generate conic forms for optimization problems and to interface with conic solvers, while providing comprehensive reference material for users.

Documentation Enhancements:

  • Added a new README.md with a quick start guide, detailed documentation index, tutorials, guides, and references for SymbolicAnalysis.jl.
  • Introduced atoms_table.md, a complete, source-verified table of all DCP and DGCP atoms, including domains, curvature, monotonicity, cone types, and literature references.

Conic Form and MOI Integration:

  • Imported MathOptInterface as MOI in the main module and included conic.jl and moi_bridge.jl to enable conic form generation and solver integration. [1] [2]

DCP Atom Registry Improvements:

  • Updated all DCP atoms in src/atoms.jl to register the appropriate MOI cone type (e.g., MOI.Reals, MOI.SecondOrderCone, MOI.PositiveSemidefiniteConeTriangle, MOI.LogDetConeTriangle, MOI.ExponentialCone, etc.), enabling automatic conic form conversion for compatible atoms. [1] [2] [3] [4] [5] [6] [7]
  • Fixed the symbolic registration of matrix_frac and clarified comments on the convexity of p-norms for p < 1.

These changes significantly improve both the usability and extensibility of SymbolicAnalysis.jl for convex optimization and geodesically convex programming.


Most Important Changes:

Documentation Improvements

  • Added docs/README.md with quick start, tutorials, guides, and references for SymbolicAnalysis.jl, including usage examples and links to further documentation.
  • Added docs/atoms_table.md, a comprehensive, source-verified reference table of all supported DCP and DGCP atoms, including their properties and cone annotations.

Conic Form and Solver Integration

  • Imported MathOptInterface as MOI in the main module and included conic.jl and moi_bridge.jl to enable conic form generation and solver interoperability. [1] [2]

DCP Atom Registry and Cone Annotations

  • Updated all DCP atoms in src/atoms.jl to include explicit MOI cone annotations, allowing for automatic mapping of atoms to their conic representations for use with conic solvers. [1] [2] [3] [4] [5] [6] [7]
  • Fixed symbolic registration for matrix_frac and clarified the convexity domain for p-norm atoms.

Vaibhavdixit02 and others added 9 commits February 14, 2026 10:10
This commit addresses technical and numerical comments from DGCP paper reviewers:

Experiments Added/Updated:
- dcp_dgcp_comparison.jl: Add timing comparison section showing DGCP overhead
  is minimal (<5x) compared to DCP-style analysis (Reviewer #1, #2)
- expert_examples.jl: Fix operator precedence bugs in expression construction
- non_gconvex_examples.jl: 6 test cases for non-g-convex identification
- convergence_comparison.jl: Euclidean BFGS vs Riemannian GD/CG comparison
- extended_benchmark.jl: AST complexity metrics + verification timing

Documentation Added:
- docs/atoms_table.md: Comprehensive table of all DGCP atoms with domains,
  curvatures, monotonicities, and literature references
- docs/porting_guide.md: Complete Python (SymPy) and Matlab implementation
  guide for porting DGCP to other languages

Validation:
- test/experiments/VALIDATION_REPORT.md: Cross-check of experiments against
  specific reviewer comments

Addresses: Tech Editor #2-4, Reviewer #1.3-9, Reviewer #2.1-3
Introduce MathOptInterface and JuMP as dependencies to enable solver-ready
conic formulations from DCP/DGCP-verified expressions.

- Extend makerule/makegrule with optional `cone` kwarg for MOI cone types
- Annotate all ~50 DCP atoms and ~23 GDCP atoms with MOI cone mappings
  (ExponentialCone, SecondOrderCone, PSD, RotatedSOC, etc.)
- Add src/conic.jl: ConicFormulation struct and to_conic_form() that walks
  expression trees bottom-up, introducing epigraph variables and cone
  constraints per atom
- Add src/moi_bridge.jl: to_jump_model() and to_moi_model() converters
  for solver dispatch
- Add test/conic_tests.jl with 463 tests covering cone annotations,
  conic form generation, and MOI/JuMP model creation
Remove string message arguments from @test calls — the @test macro
does not accept a message string as a second positional argument.
…xperiments

- Rewrite conic.jl with vector-valued ConeConstraint/ConicConstraintTerm structs,
  thread-safe ConicContext, affine expression flattening, and atom reformulations
  for max, min, sqrt, inv, rel_entr, quad_over_lin
- Rewrite moi_bridge.jl with generic constraint dispatch replacing per-cone if-elseif
- Expand conic_tests.jl with 517 tests covering new structs, atoms, and MOI bridge
- Replace Plots.jl with CairoMakie for publication-quality figures
- Add experiment scripts and CSV results for paper revision
- Update .gitignore to exclude generated assets
These paper revision files were committed by mistake. They are now
gitignored along with .tex files and the _MPC_v2__DGCP/ directory.
Documentation:
- Add docs/README.md with quick start and documentation index
- Add docs/tutorials/dgcp_tutorial.md covering full DGCP workflow
- Add docs/tutorials/conic_form_tutorial.md for conic form and MOI bridge
- Add docs/examples.md with 6 worked optimization problem examples
- Update docs/atoms_table.md with cone annotation column

Code quality:
- Add DCP verification guard in to_conic_form (warns on UnknownCurvature)
- Fix sum_largest test expectation to match corrected implementation
- Strengthen non_gconvex_examples assertion from any() to all()
- Add JIT warmup to expert_examples timing measurements
- Add reproducibility seeds to 4 experiment files

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR expands SymbolicAnalysis.jl with explicit MOI cone annotations for atoms, adds conic-form generation + MOI/JuMP bridging, and significantly extends documentation and experiment/test coverage around DGCP/DCP and conic integration.

Changes:

  • Add MOI cone metadata to DCP/GDCP atom registries and expose conic-form + MOI bridge modules from the main package.
  • Introduce conic-form generation tests (and additional DGCP-focused experiments/tests).
  • Add/expand documentation (tutorials, examples, and a comprehensive atoms reference table).

Reviewed changes

Copilot reviewed 36 out of 39 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/runtests.jl Adds a new testset running conic/MOI integration tests.
test/interface_tests.jl Fixes sum_largest expectation and clarifies comments.
test/experiments/run_all_experiments.jl Adds an experiment driver that runs/exports multiple DGCP/DCP experiments and plots.
test/experiments/results/timing_comparison.csv Adds checked-in timing results output.
test/experiments/results/scope_comparison.csv Adds checked-in scope comparison results output.
test/experiments/results/scaling_analysis.csv Adds checked-in scaling results output.
test/experiments/results/mle_experiment.csv Adds checked-in MLE experiment results output.
test/experiments/results/extended_benchmark.csv Adds checked-in benchmark results output.
test/experiments/results/expert_examples.csv Adds checked-in “expert examples” results output.
test/experiments/non_gconvex_examples.jl Adds non-g-convex identification experiment + tests.
test/experiments/mle_experiment.jl Adds MLE experiment + tests for SPD manifold objectives.
test/experiments/generate_figures.jl Adds a script to generate publication figures from CSV experiment outputs.
test/experiments/extended_benchmark.jl Adds extended benchmark with AST complexity metrics + tests.
test/experiments/expert_examples.jl Adds “expert vs DGCP automated verification” experiment + tests.
test/experiments/dcp_dgcp_comparison.jl Adds scope/timing/scaling comparisons between DCP and DGCP (optionally via Convex.jl).
test/experiments/convergence_comparison.jl Adds optimization convergence comparison (Euclidean vs Riemannian solvers) + tests.
test/experiments/canonicalization_tests.jl Adds tests for the new DGCP-aware canonicalization pass.
test/dgp.jl Adjusts DGCP tests and adds a “DGCP reduces to DCP” testset.
test/conic_tests.jl Adds tests for cone annotations, conic form generation, and MOI/JuMP model bridging.
test/benchmark.jl Adds a standalone DGCP benchmarking script (plots + CSV export).
test/Project.toml Adds test/experiment dependencies (MOI/JuMP/DataFrames/CSV/Makie/etc.).
src/rules.jl Adds optional cone field to DCP rule registration.
src/moi_bridge.jl Introduces conversion from ConicFormulation to MOI/JuMP models and solution extraction.
src/gdcp/spd.jl Adds MOI cone metadata to SPD GDCP atoms; adjusts logdet sign annotation.
src/gdcp/lorentz.jl Adds MOI cone metadata to Lorentz GDCP atoms and refines lorentz barrier definition.
src/gdcp/gdcp_rules.jl Adds optional cone field to GDCP rules and adjusts g-curvature propagation logic.
src/canon.jl Adds a DGCP-aware canonicalization module with optional extended rewriting.
src/atoms.jl Adds MOI cone metadata across DCP atoms; fixes matrix_frac symbolic registration and sum_largest indexing.
src/SymbolicAnalysis.jl Imports MOI and includes new conic.jl + moi_bridge.jl modules.
docs/tutorials/dgcp_tutorial.md Adds DGCP workflow tutorial and troubleshooting guidance.
docs/tutorials/conic_form_tutorial.md Adds conic form + MOI/JuMP bridge tutorial and cone mapping guidance.
docs/examples.md Adds worked SPD manifold examples with DGCP verification walkthroughs.
docs/atoms_table.md Adds a source-verified atom registry table incl. curvature/monotonicity/cone metadata.
docs/README.md Adds documentation index and quick start entrypoint.
Project.toml Adds MOI/JuMP as package dependencies.
.gitignore Ignores generated artifacts (plots/doc exports) and experiment assets.
Comments suppressed due to low confidence (1)

test/experiments/results/timing_comparison.csv:1

  • These look like generated experiment outputs being committed into the repo (test/experiments/results/*.csv). This can become stale and create noisy diffs when regenerated on different machines/Julia versions. Consider either generating these files in CI/release workflows instead of checking them in, or moving them under an ignored artifacts/output directory and documenting how to reproduce them.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gdcp/gdcp_rules.jl Outdated
Comment on lines 191 to 228
if f_curvature == Convex || f_curvature == Affine
if all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
m = get_arg_property(f_monotonicity, i, args)
# @show arg
if arg_curv == GConvex
m == Increasing
elseif arg_curv == GConcave
m == Decreasing
else
arg_curv == GLinear
end
convex_ok = all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
m = get_arg_property(f_monotonicity, i, args)
if arg_curv == GConvex
m == Increasing
elseif arg_curv == GConcave
m == Decreasing
elseif arg_curv == GLinear
m == Increasing || m == Decreasing || m == GIncreasing || m == GDecreasing
else
false # GUnknownCurvature
end
end
if convex_ok
return GConvex
else
return GUnknownCurvature
end
elseif f_curvature == Concave || f_curvature == Affine
if all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
m = f_monotonicity[i]
if arg_curv == GConcave
m == Increasing
elseif arg_curv == GConvex
m == Decreasing
else
arg_curv == GLinear
end
elseif f_curvature == Concave
concave_ok = all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
m = get_arg_property(f_monotonicity, i, args)
if arg_curv == GConcave
m == Increasing
elseif arg_curv == GConvex
m == Decreasing
elseif arg_curv == GLinear
m == Increasing || m == Decreasing || m == GIncreasing || m == GDecreasing
else
false # GUnknownCurvature
end
return GConcave
else
return GUnknownCurvature
end
elseif f_curvature == Affine
if all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
arg_curv == GLinear
end
return GLinear
if concave_ok
return GConcave
else
return GUnknownCurvature
end

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

Affine functions are being treated as GConvex via f_curvature == Convex || f_curvature == Affine, and there is no longer a branch that can return GLinear. This will misclassify affine subexpressions and can also break downstream rules that rely on linearity. Fix by handling Affine separately: require all arguments be GLinear (and accept any monotonicity), then return GLinear; keep the Convex branch restricted to Convex only.

Copilot uses AI. Check for mistakes.
Comment thread src/canon.jl
Comment on lines +75 to +84
function canonize_extended(ex)
ex = canonize(ex) # First apply core rules

extended_rules = [
# logdet(inv(X)) → -logdet(X)
@rule LinearAlgebra.logdet(inv(~X)) => -LinearAlgebra.logdet(~X)

# log(a * b) → log(a) + log(b) for positive sub-expressions
@rule log(~a * ~b) => log(~a) + log(~b)
]

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

The rewrite log(a*b) => log(a)+log(b) is not generally valid without additional assumptions (e.g., a > 0 and b > 0, plus avoiding branch-cut issues). Even though canonize_extended is optional, this rule can silently change semantics and potentially turn non-verifiable expressions into "verifiable" ones incorrectly. Consider removing this rule, or guarding it with a predicate that proves both factors are strictly positive (e.g., using sign propagation results) before applying it.

Copilot uses AI. Check for mistakes.
Comment thread src/moi_bridge.jl Outdated
Comment on lines +59 to +66
# Scalar equality: single term, expression == 0
@assert length(c.terms) == 1
ct = c.terms[1]
expr = JuMP.AffExpr(ct.constant)
for (v, coeff) in zip(ct.vars, ct.coeffs)
JuMP.add_to_expression!(expr, coeff, jump_vars[v])
end
JuMP.@constraint(model, expr == 0)

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

For MOI.EqualTo, the bridge currently hard-codes the RHS to 0 (expr == 0) and ignores the set value carried by c.cone (e.g., MOI.EqualTo(α)). This makes the bridge incorrect if any equality constraints are generated with a nonzero RHS. Use c.cone.value (or equivalent accessor) as the RHS in the JuMP constraint, or normalize the affine function so MOI.EqualTo(0) is always used consistently.

Suggested change
# Scalar equality: single term, expression == 0
@assert length(c.terms) == 1
ct = c.terms[1]
expr = JuMP.AffExpr(ct.constant)
for (v, coeff) in zip(ct.vars, ct.coeffs)
JuMP.add_to_expression!(expr, coeff, jump_vars[v])
end
JuMP.@constraint(model, expr == 0)
# Scalar equality: single term, expression == c.cone.value
@assert length(c.terms) == 1
ct = c.terms[1]
expr = JuMP.AffExpr(ct.constant)
for (v, coeff) in zip(ct.vars, ct.coeffs)
JuMP.add_to_expression!(expr, coeff, jump_vars[v])
end
rhs = c.cone.value
JuMP.@constraint(model, expr == rhs)

Copilot uses AI. Check for mistakes.
Comment thread src/moi_bridge.jl Outdated
terms = [MOI.ScalarAffineTerm(coeff, var_map[v])
for (v, coeff) in zip(ct.vars, ct.coeffs)]
func = MOI.ScalarAffineFunction(terms, ct.constant)
MOI.add_constraint(model, func, MOI.EqualTo(0.0))

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

Same issue as the JuMP bridge: the MOI bridge always uses MOI.EqualTo(0.0) regardless of the c.cone value being bridged. If ConeConstraint ever stores MOI.EqualTo(α) for α ≠ 0, this constraint will be wrong. Prefer MOI.add_constraint(model, func, c.cone) (or normalize the function constant and/or set so they match).

Suggested change
MOI.add_constraint(model, func, MOI.EqualTo(0.0))
MOI.add_constraint(model, func, c.cone)

Copilot uses AI. Check for mistakes.
Comment thread src/atoms.jl Outdated
Comment on lines +472 to +478
function dcprule(::typeof(^), x::Symbolic, i)
args = (x, i)
if isone(i)
return makerule(RealLine(), AnySign, Affine, Increasing), args
return makerule(RealLine(), AnySign, Affine, Increasing; cone = MOI.Reals), args
elseif isinteger(i) && iseven(i)
return makerule(RealLine(), Positive, Convex, increasing_if_positive), args
return makerule(RealLine(), Positive, Convex, increasing_if_positive;
cone = MOI.SecondOrderCone), args

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

The cone annotation for even integer powers (cone = MOI.SecondOrderCone) is not a correct conic representation for epigraphs of x^p in general (and in particular it conflicts with the tutorial/table that describes x^2 via RSOC). If to_conic_form uses this metadata, it risks generating invalid conic formulations. Consider (a) special-casing i == 2 to use MOI.RotatedSecondOrderCone and (b) using MOI.PowerCone (with a correct decomposition) for other supported exponents, or leaving cone = nothing until a mathematically correct reformulation exists.

Copilot uses AI. Check for mistakes.
Comment thread src/atoms.jl Outdated
Convex,
increasing_if_positive
increasing_if_positive;
cone = MOI.SecondOrderCone # General norm cone (SOC for p=2, NormCone for general p)

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

This registers norm(x, p) for all p >= 1 but annotates the cone as MOI.SecondOrderCone, which is only appropriate for the 2-norm (and not for p=1, p=Inf, or general p). If conic conversion relies on this annotation, it will be incorrect for most p. A safer approach is to either (1) annotate cones conditionally based on p (e.g., p==1 => NormOneCone, p==2 => SecondOrderCone, p==Inf => NormInfinityCone, else nothing) or (2) keep cone = nothing and require explicit supported p-values for conic form.

Suggested change
cone = MOI.SecondOrderCone # General norm cone (SOC for p=2, NormCone for general p)
cone = nothing # Cone depends on p; conic form must handle specific p-values explicitly

Copilot uses AI. Check for mistakes.
Comment thread src/atoms.jl Outdated
Symbolics.@register_symbolic LogExpFunctions.xlogx(x::Real)
add_dcprule(xlogx, RealLine(), AnySign, Convex, AnyMono)
add_dcprule(xlogx, RealLine(), AnySign, Convex, AnyMono;
cone = MOI.ExponentialCone)

Copilot AI Feb 23, 2026

Copy link

Choose a reason for hiding this comment

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

xlogx typically has a standard conic representation via the Relative Entropy cone (e.g., in MOI: MOI.RelativeEntropyCone), and your conic tutorial later describes xlogx under RelativeEntropyCone. Annotating it as MOI.ExponentialCone is inconsistent with that and may lead to_conic_form to pick the wrong reformulation strategy. Consider updating cone to MOI.RelativeEntropyCone (and ensuring conic generation uses the correct set dimension/ordering).

Suggested change
cone = MOI.ExponentialCone)
cone = MOI.RelativeEntropyCone)

Copilot uses AI. Check for mistakes.
- Fix: pass args to _emit_atom_constraint! in conic.jl
- Rewrite porting guide to focus on CVXPY extension approach
- Add scaling analysis experiment (backs Section 4.4 complexity claims)
- Add complexity plots generator for paper figures
- Add Convex.jl and MOI comparison experiments
- Add listing screenshot generator
- Add complexity analysis and empirical scaling documentation
Comment thread src/moi_bridge.jl
end
JuMP.@constraint(model, vec_expr in c.cone)
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could potentially do something like

if c.cone isa MOI.AbstractScalarSet
    ct = only(c.terms)
    JuMP.@constraint(
        model, 
        ct.constant + sum(c * jump_vars[v] for (c, v) in zip(ct.coeffs, ct.vars)) in c.cone,
    )
else
    @assert c.cone isa MOI.AbstractVectorSet
    JuMP.@constraint(
        model, 
        [ct.constant + sum(c * jump_vars[v] for (c, v) in zip(ct.coeffs, ct.vars)) for ct in c.terms] in c.cone,
    )
end

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks Oscar, this is a great suggestion. I refactored both bridges to use the scalar-set vs vector-set pattern (with expr in c.cone in JuMP, and MOI.add_constraint(model, func, c.cone) in the raw MOI bridge), so we no longer special-case EqualTo(0) and now preserve set parameters like EqualTo(α) correctly. Just pushed in a24c54c.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 49 out of 51 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread src/canon.jl Outdated
] => conjugation(~X, ~B)

# Conjugation recognition: B'*X*B → conjugation(X, B)
@rule ((adjoint(~B) * ~X) * ~B)[Base.OneTo(size(~B, 2)), Base.OneTo(size(~B, 1))] =>

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

The canonicalization rule for recognizing B' * X * B slices the result with Base.OneTo(size(B, 2)) for rows but Base.OneTo(size(B, 1)) for columns. For a general (non-square) B, B'XB is size(B,2)×size(B,2), so this pattern likely won’t match (or is inconsistent with conjugation’s registered output size). Consider using size(B, 2) for both dimensions so canonicalization works for rectangular B too.

Suggested change
@rule ((adjoint(~B) * ~X) * ~B)[Base.OneTo(size(~B, 2)), Base.OneTo(size(~B, 1))] =>
@rule ((adjoint(~B) * ~X) * ~B)[Base.OneTo(size(~B, 2)), Base.OneTo(size(~B, 2))] =>

Copilot uses AI. Check for mistakes.
Comment thread test/limitation.jl Outdated
@@ -0,0 +1 @@

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

This file is empty (only a blank line). If it’s not intentionally reserving a filename for future work, it should be removed to avoid confusion/noise in the test suite.

Copilot uses AI. Check for mistakes.
Comment thread src/SymbolicAnalysis.jl
Comment on lines +64 to +65
include("conic.jl")
include("moi_bridge.jl")

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

Including moi_bridge.jl in the main module means JuMP is imported/loaded whenever SymbolicAnalysis is loaded, even if users only want DCP/DGCP analysis. This can materially increase install footprint and load/precompile time. Consider making the JuMP bridge an optional dependency (e.g., a Julia package extension) while keeping core MOI/conic functionality available without JuMP.

Copilot uses AI. Check for mistakes.
Comment thread src/gdcp/lorentz.jl
Comment on lines +42 to +49
add_gdcprule(
lorentz_log_barrier,
Manifolds.Lorentz,
Positive,
GConvex,
GIncreasing;
cone = MOI.ExponentialCone,
)

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

lorentz_log_barrier(p) is registered with Positive sign, but the implementation -log(-1 + p[end]) can be negative when p[end] > 2. This will cause incorrect sign metadata to be propagated. Consider changing the GDCP rule’s sign to AnySign (or tightening the function/domain so it is always nonnegative).

Copilot uses AI. Check for mistakes.
Comment on lines +219 to +225
save(
"/Users/vaibhavdixit02/SymbolicAnalysis.jl/_MPC_v2__DGCP/figures/scaling_verification.pdf",
fig1,
)
save(
"/Users/vaibhavdixit02/SymbolicAnalysis.jl/_MPC_v2__DGCP/figures/scaling_verification.png",
fig1,

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

This script saves figures to hard-coded absolute paths under /Users/..., which will fail on other machines and in CI. Please write outputs relative to the repo (e.g., joinpath(@__DIR__, "..", "..", "_MPC_v2__DGCP", "figures")) or accept an output directory argument/env var; also ensure the directory exists before saving.

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +51
# Listing 11: Square of logdet
make_listing_image(
[
"julia> @variables X[1:3, 1:3]",
" M = SymmetricPositiveDefinite(3)",
" result = analyze(logdet(X)^2, M)",
" println(result.gcurvature)",
],
["GUnknownCurvature"],
"/Users/vaibhavdixit02/SymbolicAnalysis.jl/_MPC_v2__DGCP/listing/11.png",
)

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

The output filenames are hard-coded to an absolute /Users/... path, making the script non-portable. Please switch to a path derived from @__DIR__ (or a user-supplied output directory) and create the directory if needed.

Copilot uses AI. Check for mistakes.
Comment thread docs/atoms_table.md Outdated
| `huber(x, M)` | Real | Positive | Convex | increasing_if_positive | SecondOrderCone | Literature | Grant & Boyd (2006) |
| `inv(x)` | Positive Real | Positive | Convex | Decreasing | RotatedSecondOrderCone | Literature | Grant & Boyd (2006) |
| `inv(X)` | Semidefinite | AnySign | Convex | Decreasing | PSDConeTriangle | Literature | Grant & Boyd (2006) |
| `xlogx(x)` | Real | AnySign | Convex | AnyMono | ExponentialCone | Literature | Grant & Boyd (2006) |

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

The cone type for xlogx(x) is listed as ExponentialCone, but in src/atoms.jl it is registered with cone = MOI.RelativeEntropyCone. Please update the table to match the source (or clarify if multiple reformulations are supported).

Suggested change
| `xlogx(x)` | Real | AnySign | Convex | AnyMono | ExponentialCone | Literature | Grant & Boyd (2006) |
| `xlogx(x)` | Real | AnySign | Convex | AnyMono | RelativeEntropyCone | Literature | Grant & Boyd (2006) |

Copilot uses AI. Check for mistakes.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 48 out of 50 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread src/gdcp/gdcp_rules.jl
Comment on lines +198 to +206
if f_curvature == Convex
convex_ok = all(enumerate(args)) do (i, arg)
arg_curv = find_gcurvature(arg)
m = get_arg_property(f_monotonicity, i, args)
if arg_curv == GConvex
m == Increasing
elseif arg_curv == GConcave
m == Decreasing
elseif arg_curv == GLinear

Copilot AI Mar 10, 2026

Copy link

Choose a reason for hiding this comment

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

find_gcurvature uses get_arg_property to retrieve argument monotonicity, but then compares it to the DCP enum values Increasing/Decreasing. For GDCP rules, rule.gmonotonicity is typically GIncreasing/GDecreasing, so this condition will never be true and many valid compositions will incorrectly return GUnknownCurvature. Consider normalizing monotonicity to a common enum (or accepting both Increasing|GIncreasing and Decreasing|GDecreasing) before these checks.

Copilot uses AI. Check for mistakes.
@langestefan

langestefan commented Mar 14, 2026

Copy link
Copy Markdown

hi @Vaibhavdixit02 !

I am a big fan of DCP so I have been following this PR closely!

I added a PR at #96 that adds an interactive atom table, inspired by the tables you can view at https://www.cvxpy.org/tutorial/functions/index.html.

Let me know if you want to add that to this PR, or merge it later.

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.

4 participants