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

Updating backend docstrings #2213

Merged
merged 15 commits into from
Aug 22, 2023

Conversation

Transurgeon
Copy link
Collaborator

Description

This PR attempts to add docstrings to all functions in the canonicalization backend. Notably, this includes the new NumPy backend that has recently been added in #2186 . This branch is WIP but a good chunk has already been completed, so a first review isn't out of the question. I also found a neat way to refactor indexing in the kron functions.

P.S: Adding non-trivial docstrings is actually kind of challenging, please bare with all the redundancy and reiterations that I may have introduced.

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • 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.

@CLAassistant
Copy link

CLAassistant commented Aug 17, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ phschiele
❌ Transurgeon


Transurgeon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Benchmarks that have stayed the same:

   before           after         ratio
 [fa70b28a]       [d0a6eb76]
      7.35±0s          7.54±0s     1.03  optimal_advertising.OptimalAdvertising.time_compile_problem
      3.52±0s          3.60±0s     1.02  quantum_hilbert_matrix.QuantumHilbertMatrix.time_compile_problem
      30.6±0s          31.3±0s     1.02  finance.CVaRBenchmark.time_compile_problem
     56.4±0ms         57.2±0ms     1.02  matrix_stuffing.SmallMatrixStuffing.time_compile_problem
      8.78±0s          8.90±0s     1.01  semidefinite_programming.SemidefiniteProgramming.time_compile_problem
      2.77±0s          2.80±0s     1.01  tv_inpainting.TvInpainting.time_compile_problem
      678±0ms          682±0ms     1.01  gini_portfolio.Yitzhaki.time_compile_problem
      11.4±0s          11.4±0s     1.00  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      1.77±0s          1.77±0s     1.00  simple_QP_benchmarks.LeastSquares.time_compile_problem
      46.4±0s          46.6±0s     1.00  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      3.76±0s          3.77±0s     1.00  simple_QP_benchmarks.UnconstrainedQP.time_compile_problem
      425±0ms          425±0ms     1.00  matrix_stuffing.ParamSmallMatrixStuffing.time_compile_problem
      1.86±0s          1.86±0s     1.00  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem
      24.3±0s          24.3±0s     1.00  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem
      1.24±0s          1.24±0s     1.00  simple_QP_benchmarks.ParametrizedQPBenchmark.time_compile_problem
      2.01±0s          2.00±0s     1.00  matrix_stuffing.ParamConeMatrixStuffing.time_compile_problem
      731±0ms          725±0ms     0.99  slow_pruning_1668_benchmark.SlowPruningBenchmark.time_compile_problem
      3.88±0s          3.84±0s     0.99  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      9.76±0s          9.65±0s     0.99  huber_regression.HuberRegression.time_compile_problem
      2.75±0s          2.71±0s     0.99  high_dim_convex_plasticity.ConvexPlasticity.time_compile_problem
      1.12±0s          1.11±0s     0.99  matrix_stuffing.ConeMatrixStuffingBench.time_compile_problem
      2.62±0s          2.58±0s     0.98  finance.FactorCovarianceModel.time_compile_problem
      538±0ms          529±0ms     0.98  gini_portfolio.Murray.time_compile_problem
      406±0ms          394±0ms     0.97  simple_QP_benchmarks.SimpleQPBenchmark.time_compile_problem
      1.68±0s          1.55±0s     0.92  gini_portfolio.Cajas.time_compile_problem

@SteveDiamond
Copy link
Collaborator

Great work adding these docstrings! Having docstrings sets a good pattern for future developers.

Transurgeon and others added 3 commits August 18, 2023 23:38
Co-authored-by: Philipp Schiele <44360364+phschiele@users.noreply.github.com>
[0 0 2 0],
[0 0 0 2]]]

However computing kron(x, a) directly gives us:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a comparison of normal kron(x, a) to show why reordering is necessary. I am not sure if this is a great idea as it makes the docstring very lengthy.

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 should move this to the tests and add a pointer here (which we should do in general).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll try to make some additional changes now.

cvxpy/lin_ops/canon_backend.py Outdated Show resolved Hide resolved
@phschiele phschiele marked this pull request as ready for review August 22, 2023 10:09
Copy link
Collaborator

@phschiele phschiele left a comment

Choose a reason for hiding this comment

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

Good from my side.

@phschiele
Copy link
Collaborator

Benchmarks are failing because of a new asv release. Merging here as it only added docs.

@phschiele phschiele merged commit 586ef7c into cvxpy:master Aug 22, 2023
49 of 50 checks passed
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