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

fix: remove reductions import in top-level __init__.py #2105

Merged
merged 1 commit into from
Apr 16, 2023

Conversation

akshayka
Copy link
Collaborator

Description

Reductions are stated to not be in the public API
(https://www.cvxpy.org/api_reference/cvxpy.reductions.html#disclaimer, #2104 (comment)), but they are imported in the top-level __init__.py.

This change removes that import.

Type of change

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

Reductions are stated to not be in the public API
(https://www.cvxpy.org/api_reference/cvxpy.reductions.html#disclaimer,
#2104 (comment)),
but they are imported in the top-level `__init__.py`.

This change removes that import.
@github-actions
Copy link

Benchmarks that have stayed the same:

   before           after         ratio
 [84398785]       [00daf0ef]
      31.1±0s          31.6±0s     1.02  cvar_benchmark.CVaRBenchmark.time_compile_problem
      9.57±0s          9.59±0s     1.00  huber_regression.HuberRegression.time_compile_problem
      4.11±0s          4.12±0s     1.00  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      46.7±0s          46.6±0s     1.00  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      8.68±0s          8.65±0s     1.00  optimal_advertising.OptimalAdvertising.time_compile_problem
      11.3±0s          11.3±0s     0.99  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      9.17±0s          9.10±0s     0.99  semidefinite_programming.SemidefiniteProgramming.time_compile_problem
      1.89±0s          1.87±0s     0.99  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem
      24.7±0s          24.4±0s     0.99  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem

Copy link
Collaborator

@SteveDiamond SteveDiamond left a comment

Choose a reason for hiding this comment

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

LGTM

@SteveDiamond
Copy link
Collaborator

Is it possible we put the reductions import there because it helped build the sphinx API docs? Not a reason to keep it of course.

@rileyjmurray
Copy link
Collaborator

rileyjmurray commented Apr 15, 2023 via email

@SteveDiamond
Copy link
Collaborator

I checked with git blame and it looks like it was related to the API docs:
19c5177
Anyway we can make the docs work easily enough.

@rileyjmurray
Copy link
Collaborator

rileyjmurray commented Apr 15, 2023 via email

@akshayka
Copy link
Collaborator Author

I checked with git blame and it looks like it was related to the API docs: 19c5177 Anyway we can make the docs work easily enough.

We might as well remove them from the API docs, since they're not public.

@akshayka akshayka merged commit a6e8864 into master Apr 16, 2023
@rileyjmurray
Copy link
Collaborator

@akshayka I think they should be documented on the website. Sometimes advanced users benefit from knowing what goes on under the hood. We can put them under a "private API" heading if that feels more appropriate.

@akshayka
Copy link
Collaborator Author

@akshayka I think they should be documented on the website. Sometimes advanced users benefit from knowing what goes on under the hood. We can put them under a "private API" heading if that feels more appropriate.

I think users who are that advanced should just read the source code. The risk in documenting private symbols on our website is that regular users will find these symbols and become dependent on them; people will just treat the private API as public. Similar to black.format_str: https://github.com/search?q=black.format_str&type=code, psf/black#779

That said, I don't feel strongly. I just think removing these symbols will make maintainers' lives easier.

@phschiele phschiele mentioned this pull request Jun 22, 2023
@phschiele phschiele mentioned this pull request Jul 29, 2023
@Paulnkk Paulnkk mentioned this pull request Aug 1, 2023
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.

3 participants