Opened 3 years ago

Closed 3 years ago

#24178 closed defect (wontfix)

Make arith versions of some functions default, dispatching to symbolic

Reported by: rws Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: basic arithmetic Keywords:
Cc: jdemeyer, vdelecroix Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/make_arith_versions_of_some_functions_default__dispatching_to_symbolic (Commits, GitHub, GitLab) Commit: 4bc93d0528041b6db0c3dd556b1cd6da28a0724c
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

Of many functions there are two versions with the same name---in sage/arith and in sage/functions, examples are binomial and factorial. On startup the latter versions overwrite the former because of order of import. That creates problems both with documentation, different interface, and different behavior expected. For example in #14723 the problem of binomial(Qp(5)(8),2) could not be resolved with the sage/functions version (because there the arguments have restrictions), although the sage/arith version would have worked perfectly. OTOH the arith version can not handle binomial(x,y) but the symbolic function version can.

The logical solution would be for all cases to

  • make the arith version the default by removing the import in sage/functions/all.py; the arith version will no longer be overwritten
  • dispatch calls to the arith version with symbolic arguments to the symbolic function version
  • copy some symbolic documentation to arith because that docstring will be shown now

The issues #22314, #17489 depend on this.

Change History (18)

comment:1 Changed 3 years ago by jdemeyer

Why should there even be two versions of every function in the first place? Shouldn't we be able to support all use cases in one function?

comment:2 Changed 3 years ago by rws

I wanted to have the code completely in functions but #14723 had its problems. See also #17489.

comment:3 Changed 3 years ago by rws

I could also naively ask, if everything coerces into SR why can't symbolic functions by default coerce every argument?

comment:4 Changed 3 years ago by rws

I mean you implemented the symbolic function machinery, and it's fine if you step back and say, let others go ahead now. But you could at least review attempts to fix your design decisions.

comment:5 Changed 3 years ago by rws

Alternatively, if NOT everything coerces into SR then the symbolic binomial interface is more restricted than the arith binomial interface, and thus the arith version should be default, calling the symbolic version.

comment:6 Changed 3 years ago by rws

  • Description modified (diff)
  • Summary changed from Make arith versions of functions default, dispatching to symbolic to Make arith versions of some functions default, dispatching to symbolic

comment:8 Changed 3 years ago by rws

  • Description modified (diff)

comment:9 Changed 3 years ago by rws

  • Branch set to u/rws/make_arith_versions_of_some_functions_default__dispatching_to_symbolic

comment:10 Changed 3 years ago by git

  • Commit set to 669cfff0a419c5f08e010f76ff0d33eea235c97d

Branch pushed to git repo; I updated commit sha1. New commits:

669cfff24178: changes to arith and doctests

comment:11 Changed 3 years ago by git

  • Commit changed from 669cfff0a419c5f08e010f76ff0d33eea235c97d to 4bc93d0528041b6db0c3dd556b1cd6da28a0724c

Branch pushed to git repo; I updated commit sha1. New commits:

4bc93d024178: code simplification

comment:12 Changed 3 years ago by rws

  • Authors set to Ralf Stephan
  • Cc vdelecroix added
  • Status changed from new to needs_review

This should resolve #17489 and #22314 in one go.

Doctests pass in arith and all the symbolics and function directories but let's see.

Remaining issues like merging of documentation and deprecation of the obscure binomial._binomial_sym, binomial._eval_, binomial._evalf_ is stuff for other tickets.

Please review.

comment:13 follow-up: Changed 3 years ago by vdelecroix

Please don't do this. The point of having the functions the way they are in arith is because it is fast. Having each time an extra import, try/except, etc is a pain.

comment:14 Changed 3 years ago by rws

Not changing the arith/ versions means writing a global interface function elsewhere. I'm fine with that.

comment:15 in reply to: ↑ 13 Changed 3 years ago by chapoton

Replying to vdelecroix:

Please don't do this. The point of having the functions the way they are in arith is because it is fast. Having each time an extra import, try/except, etc is a pain.

I agree with Vincent. Keep the fast functions as they are now.

comment:16 Changed 3 years ago by rws

Nothing wrong with a fast arithmetic function version taking integer arguments, for special needs (and that's why they needed to be imported for some time now). Note that arith.misc.binomial does a lot of dispatching attempts and parent queries that do not match what you just argued.

Because the global entry functions in functions.other restrict the argument type, an unrestricted version is needed. Like as in log this can be put right next to the function class as a Python function. This would then carry a documentation overview.

comment:17 Changed 3 years ago by rws

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

The original issue won't be fixed.

comment:18 Changed 3 years ago by vdelecroix

  • Resolution set to wontfix
  • Status changed from positive_review to closed

closing positively reviewed duplicates

Note: See TracTickets for help on using tickets.