Opened 2 years ago
Closed 21 months ago
#24758 closed defect (fixed)
py3: minor fixes to sage.functions
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.5 |
Component: | python3 | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 3719f2e (Commits) | Commit: | 3719f2e3075f2ae2028db2f1eb946c8175992f25 |
Dependencies: | Stopgaps: |
Description
Two minor fixes to some code in sage.functions
, both coincidentally involving min/max, but in unrelated ways.
Change History (17)
comment:1 Changed 2 years ago by
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
- Status changed from needs_review to needs_work
This is not a harmless change:
- return self.eval_helper(max_symbolic, builtin_max, None, args) + return self.eval_helper(max_symbolic, builtin_max, float('-inf'), args)
Instead, I think you just need to handle None
in eval_helper
. In particular in this line:
res = builtin_f(res, x)
Second, I would just remove the doctests involving None
where you added # py2
. Those tests seem rather silly. But it's fine if you want to keep them.
Everything else is fine.
comment:4 Changed 2 years ago by
I agree the None
tests are a bit silly, but they did help catch this issue in the first place (they won't in the future due to the # py2
addition, but I think I'll keep them). I agree otherwise.
comment:5 Changed 2 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:6 Changed 2 years ago by
But on the other hand, there is already float('inf')
in the min_symbolic
So this ticket is good for coherence..
comment:7 Changed 2 years ago by
I see that now, and I think it may be where I got the idea in the first place. But in principle float('inf')
doesn't necessarily always make sense here, so it would be better to use None
but in a way that makes sense on Python 3.
comment:8 Changed 2 years ago by
one should also use r"\max"
in front of latex_name="\max"
and the same for min
comment:9 Changed 2 years ago by
- Milestone changed from sage-8.3 to sage-8.4
I believe this issue can reasonably be addressed for Sage 8.4.
comment:10 Changed 2 years ago by
changes to src/sage/functions/wigner.py have been moved to #26055
comment:11 Changed 22 months ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:12 Changed 21 months ago by
What should we do with this one ?
comment:13 Changed 21 months ago by
Finish it, I guess. I'll take a look.
comment:14 Changed 21 months ago by
- Commit changed from 7a885113114653087010912e44b11a21c36ff578 to 3719f2e3075f2ae2028db2f1eb946c8175992f25
comment:15 Changed 21 months ago by
- Status changed from needs_work to needs_review
I think this is better now. This solution works on Python 2 and 3.
comment:16 Changed 21 months ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, let it be
comment:17 Changed 21 months ago by
- Branch changed from u/embray/python3/sage-functions to 3719f2e3075f2ae2028db2f1eb946c8175992f25
- Resolution set to fixed
- Status changed from positive_review to closed
I see. In Python 2,
range()
automatically callsint()
on the arguments:Python 3 uses
__index__
which is obviously better.