-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Min/Max now supports sets as argument #22572
base: master
Are you sure you want to change the base?
Conversation
✅ Hi, I am the SymPy bot (v162). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like: This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10. Click here to see the pull request description that was parsed.
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[907895ac] [c551272c]
+ 7.43±0.3ms 11.2±0.8ms 1.50 matrices.TimeMatrixPower.time_Case1
- 4.94±0.03s 357±6ms 0.07 polygon.PolygonArbitraryPoint.time_bench01
+ 4.09±0.1ms 6.70±0.4ms 1.64 solve.TimeMatrixOperations.time_det(4, 2)
+ 3.95±0.3ms 6.37±0.1ms 1.61 solve.TimeMatrixOperations.time_det_bareiss(4, 2)
+ 40.9±2ms 69.4±0.8ms 1.70 solve.TimeMatrixSolvePyDySlow.time_linsolve(1)
Full benchmark results can be found as artifacts in GitHub Actions |
def test_issue_6899(): | ||
from sympy.core.function import Lambda | ||
x = Symbol('x') | ||
eqn = Lambda(x, x) | ||
assert eqn.func(*eqn.args) == eqn | ||
|
||
|
||
def test_MinMax_with_Sets(): | ||
assert Max(Range(3)) == 2 |
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.
I think this might be conflating ideas of objects and object elements. In a similar way we needed to deistinguish between FiniteSet(FiniteSet(1, 2)) != FiniteSet(1, 2)
. When the user writes Max(a, b)
then a
and b
must be compared, not the elements within. If there is code that needs to find the largest item in sets then it should be max(i.sup for i in sets)
.
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.
I think max(Range)
is ok but have doubts about max(Range, FiniteSet)
. Note how the builtin behaves:
>>> max([1,2,3],[2])
[2]
So it returned the element that had the largest first difference. And mixed comparisons fail:
>>> max([1,2,3],[2],7)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: '>' not supported between instances of 'int' and 'list'
OK! I can see the point of not allowing So I think that it does make sense to have a "max"-function which allows a single set (and, hence, takes the maximum set member). If that then leads to a different class being executed, so be it. More work for limited benefit. (This means that while The whole point is to be able to manipulate these types of expressions without having to do additional coding, but to benefit from the built-in support. As an added benefit it will generate mathematically correct/valid output for intermediate steps. |
What about |
Note, too, that FiniteSet can hold FiniteSets, so even |
References to other Issues or PRs
Closes #22568
Brief description of what is fixed or changed
It is now possible to use sets as arguments to
Max
andMin
. I needed a way to write something likeMax(imageset(Lambda(x, f(x), Range(J))
and later on replace J with an integer.
Although this is not yet supported as it doesn't seem possible to convert
Range
toFiniteSet
at will, see #22571, it is a step in the right direction.Other comments
Release Notes
Max
andMin
now acceptSet
arguments.