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:  sageduplicate/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: 
Description (last modified by )
Of many functions there are two versions with the same namein 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
Change History (18)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
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
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
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
 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:7 Changed 3 years ago by
comment:8 Changed 3 years ago by
 Description modified (diff)
comment:9 Changed 3 years ago by
 Branch set to u/rws/make_arith_versions_of_some_functions_default__dispatching_to_symbolic
comment:10 Changed 3 years ago by
 Commit set to 669cfff0a419c5f08e010f76ff0d33eea235c97d
Branch pushed to git repo; I updated commit sha1. New commits:
669cfff  24178: changes to arith and doctests

comment:11 Changed 3 years ago by
 Commit changed from 669cfff0a419c5f08e010f76ff0d33eea235c97d to 4bc93d0528041b6db0c3dd556b1cd6da28a0724c
Branch pushed to git repo; I updated commit sha1. New commits:
4bc93d0  24178: code simplification

comment:12 Changed 3 years ago by
 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 followup: ↓ 15 Changed 3 years ago by
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
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
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
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
 Milestone changed from sage8.2 to sageduplicate/invalid/wontfix
 Status changed from needs_review to positive_review
The original issue won't be fixed.
comment:18 Changed 3 years ago by
 Resolution set to wontfix
 Status changed from positive_review to closed
closing positively reviewed duplicates
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?