Opened 12 months ago

Closed 4 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 12 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 12 months ago by jdemeyer

I see. In Python 2, range() automatically calls int() on the arguments:

sage: range(1.5, 6.5)
[1, 2, 3, 4, 5]

Python 3 uses __index__ which is obviously better.

comment:3 Changed 12 months ago by jdemeyer

  • 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 12 months ago by embray

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 10 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:6 Changed 8 months ago by chapoton

But on the other hand, there is already float('inf') in the min_symbolic

So this ticket is good for coherence..

comment:7 Changed 8 months ago by embray

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 8 months ago by chapoton

one should also use r"\max" in front of latex_name="\max" and the same for min

comment:9 Changed 7 months ago by embray

  • 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 5 months ago by chapoton

changes to src/sage/functions/wigner.py have been moved to #26055

comment:11 Changed 4 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:12 Changed 4 months ago by chapoton

What should we do with this one ?

comment:13 Changed 4 months ago by embray

Finish it, I guess. I'll take a look.

comment:14 Changed 4 months ago by git

  • Commit changed from 7a885113114653087010912e44b11a21c36ff578 to 3719f2e3075f2ae2028db2f1eb946c8175992f25

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fb835c5Fix some failures due to comparisons with None, which don't work on Python 2
3719f2eExplicit conversions of min() and max() outputs to int for passing as arguments to range()

comment:15 Changed 4 months ago by embray

  • 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 4 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be

comment:17 Changed 4 months ago by vbraun

  • Branch changed from u/embray/python3/sage-functions to 3719f2e3075f2ae2028db2f1eb946c8175992f25
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.