intermediate complex expression in real functions make many plot functions fail

[jasongrout]

All of the following plots fail

x, y = SR.var('x y')
contour_plot(abs(x+i*y), (x,-1,1), (y,-1,1))
density_plot(abs(x+i*y), (x,-1,1), (y,-1,1))
plot3d(abs(x+i*y), (x,-1,1),(y,-1,1))
streamline_plot(abs(x+i*y), (x,-1,1),(y,-1,1))

with

TypeError: unable to coerce to a real number

The culprit is the call to setup_for_eval_on_grid (from sage/plot/misc.py) that tries to compile the symbolic expression with fast_float. But since the expression involves an intermediate complex number the compilation fails. This can be tested with any of the two

fast_float(abs(x + i*y), x, y)
fast_callable(abs(x + i*y), vars=[x,y])

The function compilation succeeds if we ask for a complex function instead

fast_callable(abs(x + i*y), vars=[x,y], domain=complex)

See also this question on ask.sagemath.org.

CC: @orlitzky @egourgoulhon @JungMath

Component: graphics

Author: Michael Orlitzky

Branch: 6531609

Reviewer: Dima Pasechnik

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

[jasongrout]

comment:1

#5572 will help solve this.

[embray]

comment:9

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

[mkoeppe]

Dependencies: #32234

[orlitzky]

Author: Michael Orlitzky

[orlitzky]

Branch: u/mjo/ticket/8450

[orlitzky]

Commit: a773fd3

[orlitzky]

comment:11

This is an "easy fix" but causes a cascade of other issues because removing domain=float turns a lot of things that used to be Nan into TypeError, ValueError, ZeroDivisionError, etc.

I'm still running a ptestlong on this, but everything under sage/plot now passes. Here are some thoughts:

1. Do we really want to support passing (for example) the integer zero as a function to be plotted? We have doctests that check things like plot(ZZ(0), x, 0, 1). Supporting this requires special cases in the code, and IMO just perpetuates confusion about the difference between the integer 0 and the function const0.
2. We now have "try to evaluate this as a real number, and return NaN (or skip it) if we can't" code in at least five places. Should this be made consistent, or (better yet) factored out? I've made it so that the doctests pass, but in some places we catch only e.g. TypeError, while in others we check for a nice long list.
3. This needs a careful review, since it can change the appearance of plots. There was some other (now fixed) bug involved as you can see from the commit list, but for example, this fix accidentally broke list_plot.
4. I don't really like using try/except in fast loops. Is there a better way to fix the doctest failures that the first commit caused and that the later ones fix?

[dimpase]

comment:45

lgtm

[slel]

comment:46

Class FastCallablePlotWrapper, method __call__,
test block needs double-colon:

-        Evaluation never fails and always returns a ``float``:
+        Evaluation never fails and always returns a ``float``::

It might also be worth checking why all patchbots
report pyflakes errors.

[sagetrac-git]

Changed commit from 25db6da to 5f4e2fb

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

5f4e2fb

Trac #8450: add a missing colon to a doctest.

[orlitzky]

comment:48

Thanks, setting back to positive review as the last commit is trivial.

I looked at the pyflakes failures, but the logs all say that everything is OK.

[sagetrac-git]

Changed commit from 5f4e2fb to 6531609

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

6531609

Trac #8450: drop superfluous set_random_seed() call.

[orlitzky]

comment:50

Also deleted a set_random_seed() call from a doctest that we don't need to add any more.

[slel]

comment:51

Setting milestone to 9.6 now that 9.5 is out.

[vbraun]

Changed branch from u/mjo/ticket/8450 to 6531609

[kcrisman]

Changed commit from 6531609 to none

[kcrisman]

comment:53

Thanks for this fix, it will be very helpful!