-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
cvxpy v1.3.0 #75
cvxpy v1.3.0 #75
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
recipe/meta.yaml
Outdated
- cvxpy.reductions.complex2real.atom_canonicalizers | ||
- cvxpy.reductions.complex2real.canonicalizers | ||
- cvxpy.reductions.dcp2cone | ||
- cvxpy.reductions.dcp2cone.atom_canonicalizers | ||
- cvxpy.reductions.eliminate_pwl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please double-check that we're still covering most of the public API here? I see a bunch of new modules have been added over time that aren't reflected here here (yet).
Also, the sole rename of complex2real.{atom_canonicalizers -> canonicalizers}
seems inconsistent, given that all other *.atom_canonicalizers
seem to remain as-is. I also left a comment on the respective upstream PR, feel free to take this conversation where you think it's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh-oh. Didn't think to change the recipe. The rename of complex2real.atom_canonicalizers
to complex2real.canonicalizers
is because that code path includes canonicalizing Atom
and Constraint
objects. By contrast, dcp2cone.atom_canonicalizers
indeed only handles canonicalization of Atom
objects.
Technically, it seems that https://github.com/cvxpy/cvxpy/tree/master/cvxpy/reductions/dgp2dcp/atom_canonicalizers should be renamed to dgp2dcp.canonicalizers
, since it includes Constraint canonicalization. Let me look for others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like renaming dgp2dcp.atom_canonicalizers
to dgp2dcp.canonicalizers
is the only thing we might want to do for consistency.
@SteveDiamond I think a bugfix to the 1.3.x release series is necessary to resolve the issue mentioned by @h-vetinari here #75 (comment). That being the case, we might also want to rename dgp2dcp.atom_canonicalizers
to dgp2dcp.canonicalizers
while we're at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being the case, we might also want to rename
dgp2dcp.atom_canonicalizers
todgp2dcp.canonicalizers
while we're at it.
There's also {eliminate_pwl, qp2quad_form}/atom_canonicalizers
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those only process Atom
objects, so the name is appropriate there. But I would be in favor of renaming all atom_canonicalizers
folders into canonicalizers
folders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SteveDiamond I think a bugfix to the 1.3.x release series is necessary to resolve the issue mentioned by @h-vetinari here #75 (comment).
I have a PR too: cvxpy/cvxpy#1998 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, @h-vetinari, we're trying to adopt the convention that the public API consists exclusively of functionality accessible from the top-level CVXPY namespace. We moved a bunch of functionality to that namespace with 1.3. So if recipe/meta.yaml
has semantics about the public API then we might want to make bigger changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if
recipe/meta.yaml
has semantics about the public API then we might want to make bigger changes here.
I don't know what you mean by "semantics"; the only thing it checks (well, in addition to running the test suite, which should be way more comprehensive anyway), is that the various cvxpy.*
imports work. Since those are all in the toplevel namespace, the point seems to be moot anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in a way, the import tests here capture the import API, and now show that it was changed (somewhat inconsistently) with 1.3.0 (if you unfold meta.yaml
in the diff of this PR), you'll see what I mean.
Ah, and it seems some new relative imports have not yet seen any out-of-tree testing:
|
f7c1318
to
4082545
Compare
4082545
to
8e0a41c
Compare
8e0a41c
to
9d0cac3
Compare
OK, the pypy failures are an old problem that is returning. Will fix as in cvxpy/cvxpy#1208 |
@rileyjmurray @SteveDiamond |
@h-vetinari see my comment here: #75 (comment). |
@rileyjmurray Do you mean that we should only test |
Based on this, I'm still waiting for a response if that's planned to happen (ASAP) or not. |
The atom_canonicalizers folders will be renamed in the 1.3.1 patch, which will come out next week. The public API is exclusively items in https://github.com/cvxpy/cvxpy/blob/master/cvxpy/__init__.py so renaming folders is an internal change. |
This is an understandable position, but in practice will always run into Hyrum's law. Have you considered making all non-public modules have an underscore? Compare for example how scipy dealt with a very similar situation: scipy/scipy#14360 |
In this particular case I would not worry about Hyrums Law. CVXPY has a lot
of users, but I would be very surprised if any of them depended on reaching
this far into CVXPY’s folder structure to access canonicalization code.
|
It might be true for the canonicalisation, but then again people will do whatever happens to work for them, and since people tend to prefer shorter code and thus shorter types, they will often reach as far into an import hierarchy as they need to. The only semi-universally agreed-upon rule is "underscore means private, you get broken without notice". If I understood your stance correctly, then anything beyond toplevel, e.g. From some trivial searches on github: 1 2 3 4 5 6 (and that's not including code from cvxgrp, or things that are presumably quite stable like But it's your library, so I'm not going to stand in your way. 🙃 |
These are all interesting and helpful points. But the reality is that 1.3.0 is already released on PyPi. |
We'd like to gravitate to "everything not in the top-level namespace is
private", but we're not going hardcore on that at the moment. I think it's
okay to break external code that's accessing these particular folders. For
now we'd like to get 1.3.0 out on conda-forge so that it matches what's on
PyPI.
…On Fri, Jan 6, 2023 at 3:13 PM h-vetinari ***@***.***> wrote:
but I would be very surprised if any of them depended on reaching this far
into CVXPY’s folder structure to access canonicalization code.
It might be true for the canonicalisation, but then again people will do
*whatever* happens to work for them, and since people tend to prefer
shorter code and thus shorter types, they will often reach as far into an
import hierarchy as they need to. The only semi-universally agreed-upon
rule is "underscore means private, you get broken without notice".
If I understood your stance correctly, then anything beyond toplevel, e.g. import
cvxpy.something, is subject to change at any time.
From some trivial searches
<https://github.com/search?q=%22from+cvxpy%22+-path%3Acvxpy&type=code> on
github: 1
<https://github.com/uw-mad-dash/shockwave/blob/8a44f4a27a45760520386809b695d51b665b3679/scheduler/shockwave.py#L7>
2
<https://github.com/VivekPa/OptimalPortfolio/blob/cb27cbc6f0832bfc531c085454afe1ca457ea95e/PortOpt/cvx_functions.py#L1>
3
<https://github.com/materialsvirtuallab/maml/blob/0e98a5541ff12000b3cdbffae71367a57bd4fbda/maml/apps/symbolic/_selectors_cvxpy.py#L16>
4
<https://github.com/vprusso/toqito/blob/2a799a6a9384952e2b15ca22d98d3ba66ed6d783/tests/test_channels/test_partial_transpose.py#L5>
5
<https://github.com/alnurali/cvxstoc/blob/7d327e8445574b65981b44c8e468cc1faef7476c/cvxstoc/expectation.py#L4>
6
<https://github.com/r-barnes/ecoopt/blob/d4e0099c80a09635ea96e171ea087e803b713369/src/learn_to_separate.py#L7>
(and that's not including code from cvxgrp, or things that are presumably
quite stable like cvxpy.error)
But it's your library, so I'm not going to stand in your way. 🙃
—
Reply to this email directly, view it on GitHub
<#75 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACRLIFFR6QKPTF53SVO7F7DWQ7SLVANCNFSM6AAAAAATQK3U4M>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Nothing easier than that, I just didn't want to merge without giving you a chance to respond. :) |
It is very likely that the current package version for this feedstock is out of date.
Checklist before merging this PR:
license_file
is packagedInformation about this PR:
@conda-forge-admin,
please add bot automerge
in the title and merge the resulting PR. This command will add our bot automerge feature to your feedstock.bot-rerun
label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase@conda-forge-admin, please rerun bot
in a PR comment to have theconda-forge-admin
add it for you.Pending Dependency Version Updates
Here is a list of all the pending dependency version updates for this repo. Please double check all dependencies before merging.
Dependency Analysis
We couldn't run dependency analysis due to an internal error in the bot. :/ Help is very welcome!
This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/autotick-bot/actions/runs/3834745126, please use this URL for debugging.