-
-
Notifications
You must be signed in to change notification settings - Fork 396
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 integer powers to return appropriate type instead of QuadExpr #3474
Conversation
There are quite a few failures, but it is't obvious if it's because of this PR. Here's the latest |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3474 +/- ##
=======================================
Coverage 98.09% 98.09%
=======================================
Files 37 37
Lines 5501 5506 +5
=======================================
+ Hits 5396 5401 +5
Misses 105 105
☔ View full report in Codecov by Sentry. |
To be clear, this test is the current For the UnitJuMP and SumOfSquares, it appears the new operator overloading leads to some minor bugs based on assumptions made with the old API. |
I think this PR is safe to merge.
|
What about the performance impact of no longer being type stable? Should we add a note on the performance tips page? |
It's already not type stable because of the case when |
But it is type stable in JuMP 1.14, right? |
Yes. We needed to give up type stability, or else something like For a real-world motivation for this change, see: x-ref #3106 (comment) |
I agree with the code change. I do expect at least one person to be surprised either about the change in return types or the lack of type stability. It at least warrants an explicit statement in the release notes. |
Yip. I think the release notes will need to be quite detailed anyway, because it's quite a big change. Let me start a separate PR for that and we can iterate. |
I've made a note in #3485 to mention in the changelog. |
Closes #3454
This is technically breaking, since it returns a different type from before, but I'd also argue that it is a bug fix, because
x^0
andx^1
should not return a quadratic expression.Needs: