sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.47k stars 485 forks source link

fast_callable improvements (followup for #5093) #5572

Open robertwb opened 15 years ago

robertwb commented 15 years ago

The code at #5093 is very good and ready to go in, but there are several improvements that have been suggested and agreed work on at a later date. They are posted here so we can merge and close the other ticket.

Specifically, this ticket addresses these issues:

fast_callable on list, tuple, vector, matrix arguments
fast_callable on constant arguments
fast_callable of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments
in general replaces calls to fast_float with calls to fast_callable.
Carries out the deprecation in #5413 (removes the functionality deprecated there)
fast_float sometimes returns symbolic values instead of floating-point values (see #17684, which patches density_plot to work around this bug)

Because of some of the far-reaching changes, this should probably not be merged in a point-point release.

What is not fixed:

Robert's suggestion: an option that uses a fast domain, if it's there, but ignores the domain parameter if it's not (I don't mind the idea, and the implementation is easy; what should the syntax be? Part of my problem picking a syntax is that I don't want to promise that a specialized interpreter is always faster than the Python-object interpreter, so I don't particularly want to use the word "fast" in any option names.)

The work on this ticket:

Makes #8450 easy, probably
Makes #7512 invalid, probably

See also

10087
7165
11766 -- rewrite some of fast_callable to have its own recursion stack

CC: @robertwb @TimDumol @eviatarbach @novoselt @orlitzky

Component: basic arithmetic

Author: Jason Grout

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

jasongrout commented 15 years ago

comment:2

More fast_callable improvements:

domain=float is the same as domain=RDF, but domain=complex is not the same as domain=CDF. Make domain=complex use the same interpreter as domain=CDF
if g is a callable expression, then fast_callable(g) should use g.args() for the variables, not g.variables(). Hmm...or maybe return an error if g.args() is not equal to g.variables(), since every variable really does have to be satisfied.

robertwb commented 15 years ago

comment:3

Both good points. At least g.variables() should be a subset of g.args().

jasongrout commented 14 years ago

comment:4

Replying to @jasongrout:

if g is a callable expression, then fast_callable(g) should use g.args() for the variables, not g.variables(). Hmm...or maybe return an error if g.args() is not equal to g.variables(), since every variable really does have to be satisfied.

7512 may take care of this.

jasongrout commented 14 years ago

comment:5

Attachment: improve_fast_callable.patch.gz

This is a big patch to fast_callable which

makes it work for lists/tuples like fast_float does
adds lots of _fast_callable_ methods to make it work on a lot of different constant objects (integers/rationals/RDF/RR/CDF/CC)
refactors the code a bit
in general replaces calls to fast_float with calls to fast_callable.

The patch also breaks some things---it's still a work in progress.

jasongrout commented 14 years ago

Attachment: fast_callable_complex.patch.gz

apply on top of previous patch

jasongrout commented 14 years ago

comment:6

The second patch is a broken attempt at streamlining the complex support since Cython now supports C99 complex numbers.

jasongrout commented 14 years ago

comment:8

CCing: robertwb since fast_float was his idea originally

timdumol since he's expressed interest in working on this sort of thing.

The two patches need work at this point. In particular, the improvements patch needs docs added/changed, and ptestlong needs to be run to check for breakage.

The complex number patch needs an overhaul, as I think it's completely broken at this point. The complex number patch is not necessary for resolving the issues on this ticket.

jasongrout commented 14 years ago

comment:9

8450 would be a good ticket to (trivially) fix after this ticket moves plotting solely over to fast_callable.

jasongrout commented 14 years ago

rebase to 4.4.1

jasongrout commented 14 years ago

comment:10

Attachment: improve_fast_callable.2.patch.gz

I think it might be best just to fix #7512 in this ticket.

jasongrout commented 14 years ago

Attachment: improve_fast_callable.3.patch.gz

apply instead of previous patches

jasongrout commented 14 years ago

comment:11

Still not done. A clean design still needs to happen for fast_callable on symbolics without specified variables, and the nvars option seems like a hack to make plotting work with lambda functions (since we now match the argument names of lambda functions by default).

jasongrout commented 14 years ago

comment:12

Also, something should be done to put fast_float back (and its file) as a convenience method.

jasongrout commented 14 years ago

apply instead of previous patches (now doctests in plot/*.py[x] pass)

jasongrout commented 14 years ago

Attachment: improve_fast_callable.4.patch.gz

apply instead of previous patches (now almost all doctests in plot/plot3d/ pass)

jasongrout commented 14 years ago

comment:13

Attachment: improve_fast_callable.5.patch.gz

I had to rework some of the transformation code because the returned function often did not have the same keyword arguments as the original function, which throws off plotting.

jasongrout commented 14 years ago

Attachment: improve_fast_callable.6.patch.gz

apply instead of previous patches (fixed a bunch of stuff so even more doctests pass)

jasongrout commented 14 years ago

comment:14

To delete the fast_eval.so file from the build directory (necessary so that the cython fast_eval is eliminated when testing), do:

cd $SAGE_ROOT/devel/sage/build
find . -name fast_eval.so | xargs rm

jasongrout commented 14 years ago

comment:15

Progress report: my current patch queue has the following failures on make ptestlong on Sage 4.5.2:

    sage -t  -long 4.5.2/devel/sage/sage/structure/sage_object.pyx # 1 doctests failed
    sage -t  -long 4.5.2/devel/sage/sage/ext/fast_callable.pyx # Exception from doctest framework
    sage -t  -long 4.5.2/devel/sage/sage/rings/polynomial/polynomial_element.pyx # 9 doctests failed
    sage -t  -long 4.5.2/devel/sage/sage/stats/hmm/distributions.pyx # 1 doctests failed

jasongrout commented 14 years ago

comment:16

(my patch queue is up at http://sage.math.washington.edu/home/jason/sage-4.5.2-patches and this ticket involves patches up to pickling.patch)

jasongrout commented 14 years ago

Description changed:

--- 
+++ 
@@ -14,4 +14,8 @@

 * fast_callable of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments

+The work on this ticket:

+* Makes #8450 easy, probably
+* Makes #7512 invalid, probably
+

jasongrout commented 14 years ago

Description changed:

--- 
+++ 
@@ -1,18 +1,26 @@
 The code at #5093 is very good and ready to go in, but there are several improvements that have been suggested and agreed work on at a later date. They are posted here so we can merge and close the other ticket. 

-Specifically, 
+Specifically, this ticket addresses these issues:

-Not fixed:
+* fast_callable on list,tuple,vector,matrix arguments
+
+* fast_callable on constant arguments
+
+* fast_callable of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments
+
+* in general replaces calls to fast_float with calls to fast_callable. 
+
+* Carries out the deprecation in #5413 (removes the functionality deprecated there)
+
+Because of some of the far-reaching changes, this should probably not be merged in a point-point release.
+
+What is not fixed:

 * Robert's suggestion: an option that uses a fast domain, if it's there, but ignores the domain parameter if it's not (I don't mind the idea, and the implementation is easy; what should the syntax be? Part of my problem picking a syntax is that I don't want to promise that a specialized interpreter is always faster than the Python-object interpreter, so I don't particularly want to use the word "fast" in any option names.)

-* fast_callable on list,tuple,vector,matrix arguments

-* any interaction with #5413 (my plan is to wait until either #5093 or #5413 gets a positive review, then fix the other one to match)

-* fast_callable on constant arguments (waiting for a spec from Jason!)

-* fast_callable of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments

 The work on this ticket:

jasongrout commented 14 years ago

Author: Jason Grout

jasongrout commented 14 years ago

Description changed:

--- 
+++ 
@@ -27,3 +27,7 @@
 * Makes #8450 easy, probably
 * Makes #7512 invalid, probably

+See also
+* #10087
+* #7165
+

kcrisman commented 13 years ago

comment:20

What is the status of switching to fast_callable? There seem to be a number of tickets which would benefit, not to mention the fact that fast_float has old-style documentation which looks bad in the command line :) Plus, if fast_float is to be deprecated (even though it seems to use fast_callable under the hood) it would be helpful to start that process.

Anyway, just curious.

jasongrout commented 13 years ago

comment:21

A long time ago I worked on the patches you see here; I believe that all of my work is here, anyway. I probably won't have time to work on this this summer, due to notebook enhancements. I'd like to make this one of the projects for next summer if someone hasn't beaten me to it by then.

jasongrout commented 13 years ago

comment:22

The problem is that I think my patch is too big and needs to be broken down into smaller patches that change less at each step. It's been a long time, but I think I'm remembering that right. Anyways, as noted above, my patch queue is up on sage.math and anyone is welcome to work on it.

jasongrout commented 13 years ago

Description changed:

--- 
+++ 
@@ -30,4 +30,4 @@
 See also
 * #10087
 * #7165
-
+* #11766 -- rewrite some of fast_callable to have its own recursion stack

eviatarbach commented 11 years ago

comment:25

Can I just create a new patch to switch plotting to use fast_callable? Using fast_float causes big problems for plotting, namely that any point at which the function is complex at some value in the call stack fails to plot. For example, the plot of abs(log(x)) will not show up for negative x, despite the output being guaranteed to be real, since fast_float will evaluate log(x) first and choke on the complex number.

jasongrout commented 11 years ago

comment:26

Sounds fine to me. Especially if all the doctests pass (including the new one or two that you write :).

eviatarbach commented 11 years ago

comment:27

Okay, thanks. This is now #15030, with a patch up.

kcrisman commented 10 years ago

comment:30

Note that #15030 is now merged. How that does affect whatever else needs to happen here?

rwst commented 6 years ago

comment:34

Anyone interested in the deprecation of fast_float can find relevant tickets at https://trac.sagemath.org/wiki/symbolics#fast_floatdeprecation

rwst commented 6 years ago

comment:35

Without looking in detail I'd guess that implementing https://github.com/pynac/pynac/issues/8 will make ex.subs() a much faster alternative to fast_callable.

DaveWitteMorris commented 3 years ago

Description changed:

--- 
+++ 
@@ -2,15 +2,18 @@

 Specifically, this ticket addresses these issues:

-* fast_callable on list,tuple,vector,matrix arguments
+* `fast_callable` on `list`, `tuple`, `vector`, `matrix` arguments

-* fast_callable on constant arguments
+* `fast_callable` on constant arguments

-* fast_callable of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments
+* `fast_callable` of a zero multivariate polynomial returns a zero of the base ring, without paying attention to the types of the arguments

-* in general replaces calls to fast_float with calls to fast_callable. 
+* in general replaces calls to `fast_float` with calls to `fast_callable`. 

 * Carries out the deprecation in #5413 (removes the functionality deprecated there)
+
+* `fast_float` sometimes returns symbolic values instead of floating-point values (see #17684, which patches `density_plot` to work around this bug)
+

 Because of some of the far-reaching changes, this should probably not be merged in a point-point release.

@@ -30,4 +33,4 @@
 See also
 * #10087
 * #7165
-* #11766 -- rewrite some of fast_callable to have its own recursion stack
+* #11766 -- rewrite some of `fast_callable` to have its own recursion stack