Skip to content
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

Make arith versions of some functions default, dispatching to symbolic #24178

Closed
rwst opened this issue Nov 9, 2017 · 21 comments
Closed

Make arith versions of some functions default, dispatching to symbolic #24178

rwst opened this issue Nov 9, 2017 · 21 comments

Comments

@rwst
Copy link

rwst commented Nov 9, 2017

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.

CC: @jdemeyer @videlec

Component: basic arithmetic

Author: Ralf Stephan

Branch/Commit: u/rws/make_arith_versions_of_some_functions_default__dispatching_to_symbolic @ 4bc93d0

Issue created by migration from https://trac.sagemath.org/ticket/24178

@rwst rwst added this to the sage-8.2 milestone Nov 9, 2017
@jdemeyer
Copy link

jdemeyer commented Nov 9, 2017

comment:1

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?

@rwst
Copy link
Author

rwst commented Nov 9, 2017

comment:2

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

@rwst
Copy link
Author

rwst commented Nov 10, 2017

comment:3

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

@rwst
Copy link
Author

rwst commented Nov 10, 2017

comment:4

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.

@rwst
Copy link
Author

rwst commented Nov 10, 2017

comment:5

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.

@rwst

This comment has been minimized.

@rwst rwst changed the title Make arith versions of functions default, dispatching to symbolic Make arith versions of some functions default, dispatching to symbolic Mar 4, 2018
@rwst
Copy link
Author

rwst commented Mar 4, 2018

comment:7

See also #19461 comment:9

@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Mar 15, 2018

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2018

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

669cfff24178: changes to arith and doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2018

Commit: 669cfff

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2018

Changed commit from 669cfff to 4bc93d0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 15, 2018

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

4bc93d024178: code simplification

@rwst
Copy link
Author

rwst commented Mar 15, 2018

comment:12

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.

@rwst
Copy link
Author

rwst commented Mar 15, 2018

Author: Ralf Stephan

@videlec
Copy link
Contributor

videlec commented Mar 15, 2018

comment:13

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.

@rwst
Copy link
Author

rwst commented Mar 15, 2018

comment:14

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

@fchapoton
Copy link
Contributor

comment:15

Replying to @videlec:

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.

@rwst
Copy link
Author

rwst commented Mar 16, 2018

comment:16

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.

@rwst
Copy link
Author

rwst commented Mar 16, 2018

comment:17

The original issue won't be fixed.

@rwst rwst removed this from the sage-8.2 milestone Mar 16, 2018
@videlec
Copy link
Contributor

videlec commented May 18, 2018

comment:18

closing positively reviewed duplicates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants