8000 Let there be names in LP files by senhalil · Pull Request #2811 · cvxpy/cvxpy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Let there be names in LP files #2811

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Let there be names in LP files #2811

wants to merge 3 commits into from

Conversation

senhalil
Copy link
Contributor
@senhalil senhalil commented May 14, 2025 •
edited
Loading

Description

This is an unsolicited proposal. I am curious what you think and whether there is a cleaner and more automatic way to do this.

Long story short, this sets the variable/column names so that the exported LP file contains the original names set by the user.

This PR is actually on top of #2810 but I can't point at that branch as the source of that PR is not in cvxpy/cvxpy repo. Basically the first two commits are from #2810 and only the last commit is for this PR.

TODO:

  • There are two questions in NOTES
  • A small test to verify that the variable names do appear in the exported LP files
  • The lines that export the LPs need to be refactored

Issue link (if applicable): N/A

Type of change

  • New feature (backwards compatible)
  • [N/A] New feature (breaking API changes)
  • [N/A] Bug fix
  • [N/A] Other (Documentation, CI, ...)

Contribution checklist

  • [N/A] Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

@github-actions GitHub Actions
Copy link
github-actions bot commented May 14, 2025 •
edited
Loading

Benchmarks that have improved:

   before           after         ratio
 [32e53ea8]       [50a4ed74]

  •     1.09±0s          985±0ms     0.91  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem

  •     3.71±0s          3.36±0s     0.90  quantum_hilbert_matrix.QuantumHilbertMatrix.time_compile_problem

  •     351±0ms          314±0ms     0.89  slow_pruning_1668_benchmark.SlowPruningBenchmark.time_compile_problem

Benchmarks that have stayed the same:

   before           after         ratio
 [32e53ea8]       [50a4ed74]
      1.08±0s          1.15±0s     1.06  gini_portfolio.Cajas.time_compile_problem
      14.8±0s          14.8±0s     1.00  finance.CVaRBenchmark.time_compile_problem
      577±0ms          574±0ms     0.99  simple_QP_benchmarks.ParametrizedQPBenchmark.time_compile_problem
      2.38±0s          2.36±0s     0.99  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      1.21±0s          1.19±0s     0.99  finance.FactorCovarianceModel.time_compile_problem
      5.22±0s          5.13±0s     0.98  optimal_advertising.OptimalAdvertising.time_compile_problem
      24.1±0s          23.5±0s     0.98  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      764±0ms          745±0ms     0.97  matrix_stuffing.ConeMatrixStuffingBench.time_compile_problem
      1.95±0s          1.90±0s     0.97  simple_QP_benchmarks.UnconstrainedQP.time_compile_problem
      1.80±0s          1.74±0s     0.97  tv_inpainting.TvInpainting.time_compile_problem
      1.44±0s          1.40±0s     0.97  matrix_stuffing.ParamConeMatrixStuffing.time_compile_problem
      384±0ms          370±0ms     0.96  gini_portfolio.Yitzhaki.time_compile_problem
      5.03±0s          4.84±0s     0.96  huber_regression.HuberRegression.time_compile_problem
      304±0ms          292±0ms     0.96  matrix_stuffing.ParamSmallMatrixStuffing.time_compile_problem
      296±0ms          283±0ms     0.96  simple_QP_benchmarks.SimpleQPBenchmark.time_compile_problem
      5.63±0s          5.37±0s     0.95  semidefinite_programming.SemidefiniteProgramming.time_compile_problem
     48.2±0ms         45.9±0ms     0.95  matrix_stuffing.SmallMatrixStuffing.time_compile_problem
      298±0ms          283±0ms     0.95  gini_portfolio.Murray.time_compile_problem
      5.88±0s          5.55±0s     0.94  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      1.07±0s          1.01±0s     0.94  simple_QP_benchmarks.LeastSquares.time_compile_problem
      265±0ms          250±0ms     0.94  high_dim_convex_plasticity.ConvexPlasticity.time_compile_problem
      12.8±0s          12.0±0s     0.93  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem

@Transurgeon
Copy link
Collaborator

Description

This is an unsolicited proposal. I am curious what you think and whether there is a cleaner and more automatic way to do this.

I think it's a great idea, but I am not too familiar with LP files so cannot comment on how useful this feature is. Also I am not sure users would want the variable names as columns every time (so maybe it should be a flag?).

This PR is actually on top of #2810 but I can't point at that branch as the source of that PR is not in cvxpy/cvxpy repo. Basically the first two commits are from #2810 and only the last commit is for this PR.

that's ok, you can maybe merge the master branch onto this one once #2810 gets merged.

@SteveDiamond
Copy link
Collaborator

This generally looks fine. There is a lot of demand for better naming in LP files so thanks for working on this. Starting by adding the functionality for one solver interface as you've done is a good approach.

@PTNobel PTNobel force-pushed the hs/let-there-be-names-in-lp-files branch from 64720f7 to 4f459fa Compare May 16, 2025 21:24
@PTNobel
Copy link
Collaborator
PTNobel commented May 16, 2025

I went ahead and rebased the PR to clean up the diff. Can you explain the workflow for .lp files? I'm trying to better design the APIs.

@senhalil
Copy link
Contributor Author

Can you explain the workflow for .lp files?

afaik, cvxpy does not have a way to export .lp/.mps files natively and it uses the underlying solver for exporting the model.

So the workflow depends on the underlying solver (gurobi, cplex, highs, etc.) but with highs, setting write_model_file option does the trick and the solver creates a model file with the variable names it received.

If the following snippet sparked your comment, these lines were just a temporary hack, and I'll remove it now.

        # REMOVE
        solver.writeModel("highs_model.lp")
        solver.presolve()
        solver.writePresolvedModel("highs_presolved_model.lp")

@senhalil
Copy link
Contributor Author

Starting by adding the functionality for one solver interface as you've done is a good approach.

👍 Would apply be a better place for this logic? It looks like PARAM_PROB related data shenanigans are done in there.

@SteveDiamond any idea about this question:

            # NOTE: variable.variable_of_provenance() is a bit of a hack so that variables
            # created by nonneg=True etc. are named correctly.
            # Is this alright?

I suspect it should be alright (at least for nonneg=True style cases) but I don't know if variable_of_provenance is set in another case where this assumption (of using the original variable name) doesn't hold.

@senhalil
Copy link
Contributor Author

Also I am not sure users would want the variable names as columns every time (so maybe it should be a flag?).

The solvers that support variable names (at least Cplex, Gurobi, Highs) create them internally even if the user doesn't set them as far as I know, like so:

before

By passing the variable names from cvxpy space to the solver space (lp.col_names_ = var_names) we respect user's choice but if a user wants to name the variables but doesn't want to pass them to the solver then a flag would indeed be a way to go.

Variable names (for that matter, constraint names) are not only useful for model exports but they help with warnings originating from the solver when they are set correctly:

Warning:  No solution found from 1 MIP starts.
Retaining values of one MIP start for possible repair.
Warning:  Non-integral bounds for integer variables rounded.
Row 'c29993' implies binary variable 'x2283' is greater than 1.

@senhalil
Copy link
Contributor Author

I'm trying to better design the APIs.

@PTNobel was your commen about designing a general interface for model/solutions exports/writers?

@SteveDiamond
Copy link
Collaborator

Starting by adding the functionality for one solver interface as you've done is a good approach.

👍 Would apply be a better place for this logic? It looks like PARAM_PROB related data shenanigans are done in there.

@SteveDiamond any idea about this question:

            # NOTE: variable.variable_of_provenance() is a bit of a hack so that variables
            # created by nonneg=True etc. are named correctly.
            # Is this alright?

I suspect it should be alright (at least for nonneg=True style cases) but I don't know if variable_of_provenance is set in another case where this assumption (of using the original variable name) doesn't hold.

The variable_of_provenance() approach is fine.

senhalil added 2 commits May 29, 2025 15:56
@senhalil
WIP
bc58172 
- I have two questions in NOTES
- I want to add a small test to verify that the variable names do appear in the exported LP files
- I need to remove the lines that export the LPs
@senhalil
Refactor logic into a collect_column_names function and add tests
4e96c7b
@senhalil senhalil force-pushed the hs/let-there-be-names-in-lp-files branch from 4f459fa to 4e96c7b Compare May 29, 2025 13:56
@senhalil
Copy link
Contributor Author

Sorry for the radio silence folks. I refactored and cleaned up the code.

While at it:

  • I moved variable names as column names logic inside a condition that checks if the model is going to be written out (I think this if-condition ultimately can be dropped but the column name parsing code is not battle tested and I don't want to risk blocking any users)
  • It turns out Fix 2269 ERGO-Code/HiGHS#2274 is merged but not released yet. This means there is effectively no way to export the model with cvxpy+highs. Highs have a write_model_file option but highspy users were/are expected to call the writeModel function directly. I added the following snippet which can be dropped once that PR commit is released:
if options.write_model_file:
    # TODO: This part can be removed once the following HiGS PR is released:
    # https://github.com/ERGO-Code/HiGHS/pull/2274
    solver.writeModel(options.write_model_file)
  • I had to massage the variable names a bit because array names in cvxpy includes the python slice notation (i.e., , 0:# part in X[0, 0:#][0], ..., X[0, 0:#][#]) but this is not a valid column name
  • Added a general column name validation logic (that is only called if the model is going to be written out) with a corresponding unit test
  • I modified highs_qpif.py to import the column name parser from highs_conif.py to prevent the wholesale logic copy/pasta but let me know if you prefer otherwise.

And with that this PR is open for reviews 👀

@senhalil senhalil marked this pull request as ready for review May 29, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@senhalil @Transurgeon @SteveDiamond @PTNobel
0