Opened 7 years ago

Closed 5 years ago

#13246 closed defect (fixed)

Automatic exclusion of non-domain points in things like arcsec

Reported by: kcrisman Owned by: jason, was
Priority: major Milestone: sage-6.3
Component: graphics Keywords:
Cc: ddrake, ppurka, jondo, vbraun Merged in:
Authors: Punarbasu Purkayastha Reviewers: Ralf Stephan, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 384b5a2 (Commits) Commit: 384b5a23011b43380be2c434e2487121aaa7dea8
Dependencies: #16439 Stopgaps:

Description (last modified by ppurka)

From an email exchange about plotting arcsec, slightly out of order:

The immediate context for this issue is a couple of problems that include, among other things, graphing arcsec and its derivative (in one case), arcsec and arccsc (in the other). Since the range of intended functions is known, it appears that this would work if the students follow instructions (big if). But more generally, we want students to have total control over their "graphing calculator" -- at least over the input boxes we provide -- so there's nothing stopping them from entering arcsec(x/2) or something else that we can't anticipate.

The underlying problem is that the plotting code ignores everything between -1 and 1 (since those values aren't real), but then simply connects (-1,3) and (1,.2) (or so) with a line segment when it's done. I have a couple ideas for fixing this, but they won't help you in the near term.


One workaround for your @interact would be to select an option for exclude based on the function: I'm thinking of something like this:

@interact def foo(fn=textbox(), ....):

setup stuff...

if fn == 'arcsin':

exclude=[]

elif fn == 'arcsec':

exclude=[-1,1]

plot(fn, ...., exclude=exclude)

The idea is just to look at the function being plotted and set a list of points to exclude based on that. It would be tedious to code, but would work.


I think that fixing this is possible. I spent about a half hour looking at http://hg.sagemath.org/sage-main/file/9ab4ab6e12d0/sage/plot/plot.py just now and I am pretty sure that one could extract bad points like nan in generate_plot_points. Currently we just do

data = [data[i] for i in range(len(data)) if i not in exception_indices]

which means we completely ignore them, but presumably we could keep this data somehow and return it as part of generate_plot_points (not sure whether that would have to be deprecated) so that we could add that somehow to the exclude data around http://hg.sagemath.org/sage-main/file/9ab4ab6e12d0/sage/plot/plot.py#l1279

The problem is that there could be a lot of these, and that could slow things down a lot to check all of them, but checking for a "consecutive series" would also take some time.

Anyway, it's at least doable in principle, but would take some time and care to implement, making sure it didn't cause any other regressions.


Apply to devel/sage:

  1. trac_13246-rebase.patch
  2. trac_13246-parametric.patch

Attachments (5)

tmp_1.png (21.1 KB) - added by kcrisman 7 years ago.
arcsec plot
trac_13246-automatic-exclusion.patch (4.2 KB) - added by ppurka 7 years ago.
apply to devel/sage
trac_13246-rebase.patch (4.2 KB) - added by kcrisman 6 years ago.
trac_13246-parametric.patch (1.6 KB) - added by ppurka 6 years ago.
apply to devel/sage
tmp_W0a_CS copy.png (25.0 KB) - added by kcrisman 5 years ago.

Download all attachments as: .zip

Change History (75)

Changed 7 years ago by kcrisman

arcsec plot

comment:1 Changed 7 years ago by kcrisman

Here is what the bad plot looks like. This function should not be defined in the middle.

arcsec plot

comment:2 follow-up: Changed 7 years ago by kcrisman

  • Cc ppurka added

Hey ppurka, I know you like a challenge and know the plotting code pretty well... :)

comment:3 in reply to: ↑ 2 Changed 7 years ago by ppurka

Replying to kcrisman:

Hey ppurka, I know you like a challenge and know the plotting code pretty well... :)

Hehe, I will have a look. :)

This problem had come up in sage-support or sage-devel too a couple of months back.

comment:4 Changed 7 years ago by ddrake

Here's an idea I thought of that would help avoid these ugly situations. The problem is that we build our plot out of lots of tiny line segments, and we have a line segment connecting things that it should not connect. So:

Let w = xmax - xmin. After building the list of (x,y) points for the plot, go through the xs. If the difference between any two consecutive x values is "too big", don't connect them.

Since going through the list would be expensive and rarely needed, we would only do this with a keyword, say detect_gaps. If set to True, it would default to ignoring gaps of, say, 0.01*w. Otherwise, the user could specify a proportion: detect_gaps=0.05 or whatever. This procedure would only run if detect_gaps was True or a number between 0 and 1.

Does that seem reasonable? That would fix the original problem at the cost of a little speed.

comment:5 Changed 7 years ago by ppurka

@ddrake: I guess the point of this ticket is to make the exclusion automatic. Surely, there will be a slowdown if we do this automatically for every plot; the question is what will be the magnitude of the slowdown. I suspect there won't be much slowdown with the default plot_points=200.

Your solution also sounds plausible. In case we do adopt your solution, I would like it to be a part of the exclude keyword in the plot command since that is already being used to exclude user defined points from the plot. Something like exclude='automatic' for a default proportion of 0.05 or exclude=0.02 for a user defined proportion. Hopefully, this won't break the existing code and it also won't introduce any new keywords. There are already two such keywords which deal with not plotting some points - exclude and detect_poles.

Last edited 7 years ago by ppurka (previous) (diff)

comment:6 Changed 7 years ago by kcrisman

Hmm, this idea is not bad. Particularly since we pick a default step size for the x values, one could simply choose some multiple of that (like 1 or 2) as the minimum to remove in that case. There are some issues with making sure behavior is still right for generate_plot_points in non-normal cases (no recursion, etc.) but this has some promise.

comment:7 Changed 7 years ago by ppurka

Can you check many different plots with and without this patch? It should fix two things:

  1. automatically exclude invalid regions
  2. fix a bug in the exclude code. This bug can be found if you try to plot the following for instance and expect the point 1.5 to be excluded.
    plot(arcsec, -2, 2, exclude=[-1.5, 0, 0.5, 1.5])
    

comment:8 Changed 7 years ago by ppurka

Hold on.. this patch is not right. It has broken at least two plots:

plot(sin(1/x), (x, -1, 1))
plot(sin(x), (x, 0, 10), linestyle='-.')

The first one I can understand, but I don't understand the problem with the second one.

Edit: Can't reproduce this. See comment:10.

Last edited 7 years ago by ppurka (previous) (diff)

comment:9 Changed 7 years ago by kcrisman

My totally gut guess is that you're not taking adaptive recursion into account enough. Of course, one would think that would make the distance between plot points even smaller, but who knows?

I'd insert a print statement in your while loop to see what's being excluded. Also, does sin break only with the linestyle, or in general?

comment:10 Changed 7 years ago by ppurka

Actually, I am trying to reproduce it on my laptop now, and I can't reproduce the failures. I don't know what happened back then. I was manually going through each plot command in the docstrings and plotting it and verifying visually whether it was all right. For both the above examples I was getting a straight horizontal line. Now, when I am plotting it again, I don't see any anomalies. :/

I will let this patch bake for a couple more days, before I set it up for needs_review. Maybe it corrects itself automatically over time.

comment:11 Changed 7 years ago by ppurka

  • Status changed from new to needs_review

I manually and visually (!) tested all the plots in the doctests of sage/plot/plot.py and this patch hasn't adversely affected any of them. I also changed the test to 2 * (distance between two x-axis points).

Setting this up for review. If you know a bunch of ill-behaving functions like arcsec then please mention them here so that I can run more tests.

comment:12 Changed 7 years ago by ppurka

  • Description modified (diff)

comment:13 Changed 7 years ago by jdemeyer

Please fill in your real name as Author.

comment:14 Changed 7 years ago by ppurka

  • Authors set to Punarbasu Purkayastha

comment:15 Changed 7 years ago by ppurka

  • Status changed from needs_review to needs_work
  • Work issues set to fix doctests

Changed 7 years ago by ppurka

apply to devel/sage

comment:16 Changed 7 years ago by ppurka

  • Status changed from needs_work to needs_review
  • Work issues fix doctests deleted

comment:17 Changed 6 years ago by kcrisman

Notice this (unsurprisingly) needs a slight rebase.

Impressively, this also seems to work correctly with things like

sage: parametric_plot( (arcsec(x), x^2), (x,-2,2))

although

parametric_plot( (x,arcsec(x)), (x,-2,2))

would cause an error always - something that probably should be fixed, perhaps this is a good place to do so? Note that there is no helpful error message like with

WARNING: When plotting, failed to evaluate function at 99 points.

I'm trying to rack my brain thinking of where 2 times the difference between two consecutive plot points might cause a problem in the parametric case. Even

parametric_plot( (100000*arcsec(x), x), (x,-2,2),aspect_ratio='automatic')

doesn't seem to trigger additional exclusions. Do you see what I mean? One could imagine that in a parametric context there could be a very narrow parameter range, but a lot of room on the x-values and so everything would be excluded.

Changed 6 years ago by kcrisman

comment:18 Changed 6 years ago by kcrisman

Apply trac_13246-rebase.patch

Changed 6 years ago by ppurka

apply to devel/sage

comment:19 Changed 6 years ago by ppurka

  • Description modified (diff)

Thanks for rebasing. I have attached a new patch to take care of the failing parametric plot too.

Apply trac_13246-rebase.patch trac_13246-parametric.patch

comment:20 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:21 Changed 6 years ago by jondo

  • Cc jondo added

comment:22 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:23 follow-up: Changed 6 years ago by rws

  • Status changed from needs_review to needs_work

Apart from that the patch no longer applies---and I'm not sure if the changes meanwhile done in Sage possibly change the overall situation or the necessary patch---, the given doctest fails with AttributeError: 'complex' object has no attribute 'arccos'.

comment:24 in reply to: ↑ 23 Changed 6 years ago by ppurka

Replying to rws:

Apart from that the patch no longer applies---and I'm not sure if the changes meanwhile done in Sage possibly change the overall situation or the necessary patch---, the given doctest fails with AttributeError: 'complex' object has no attribute 'arccos'.

I haven't considered rebasing this patch (and a couple other patches) because there doesn't seem to be much interest in this. The issue that this patch fixes did come up in the mailing list two or three months ago, but I haven't obtained any feedback since then. As far as I know, this issue is definitely not fixed even in the latest development versions.

The problem with AttributeError is not introduced with this patch. It seems to be a problem with Sage itself. The code where it fails is

.../sage/local/lib/python2.7/site-packages/sage/functions/trig.pyc in _evalf_(self, x, parent, algorithm)
    732         if parent is float:
    733             return math.acos(1/x)
--> 734         return (1/x).arccos()
    735
    736     def _eval_numpy_(self, x):

EDIT: Sorry, I was wrong. The use of instance(parent, (float, complex)) is not correct since complex can not be converted to float apparently. In that case, there is something else wrong in Sage that is producing complex entries, when the entries were float earlier.

Last edited 6 years ago by ppurka (previous) (diff)

comment:25 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:26 Changed 6 years ago by kcrisman

Here's a recent email, though:

Guys:

I'm not sure how to comment on ticket #13246, but it appears that the problem of correctly plotting arcsec x and other functions with large domain gaps is still not solved.  Any chance for progress?

Thanks.

comment:27 Changed 6 years ago by ppurka

The error about complex entries was introduced in between 5.10.rc1 and 5.11.rc1

Earlier it used to be a ValueError and was properly handled by the plot code:

sage: from sage.plot.misc import setup_for_eval_on_grid
sage: f, d = setup_for_eval_on_grid(arcsec, [(-2, 2)])
sage: xi = -0.992725195401
sage: f(xi)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-f7215b663c19> in <module>()
----> 1 f(xi)

/usr/local/src/sage/sage-5.10.rc1.server/local/lib/python2.7/site-packages/sage/ext/interpreters/wrapper_rdf.so in sage.ext.interpreters.wrapper_rdf.Wrapper_rdf.__call__ (build/cythonized/sage/ext/interpreters/wrapper_rdf.c:1641)()

/usr/local/src/sage/sage-5.10.rc1.server/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:7218)()

/usr/local/src/sage/sage-5.10.rc1.server/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._eval_self (build/cythonized/sage/symbolic/expression.cpp:6246)()

/usr/local/src/sage/sage-5.10.rc1.server/local/lib/python2.7/site-packages/sage/functions/trig.pyc in _evalf_(self, x, parent)
    731         """
    732         if parent is float:
--> 733             return math.acos(1/x)
    734         return (1/x).arccos()
    735

ValueError: math domain error

Now it spits out AttributeError instead.

sage: from sage.plot.misc import setup_for_eval_on_grid
sage: f, d = setup_for_eval_on_grid(arcsec, [(-2, 2)])
sage: xi = -0.992725195401
sage: f(xi)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-f7215b663c19> in <module>()
----> 1 f(xi)
/usr/local/src/sage/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/ext/interpreters/wrapper_rdf.so in sage.ext.interpreters.wrapper_rdf.Wrapper_rdf.__call__ (sage/ext/interpreters/wrapper_rdf.c:1641)()
/usr/local/src/sage/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/symbolic/function.so in sage.symbolic.function.BuiltinFunction.__call__ (sage/symbolic/function.cpp:8029)()
/usr/local/src/sage/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__complex__ (sage/symbolic/expression.cpp:7805)()
/usr/local/src/sage/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._eval_self (sage/symbolic/expression.cpp:6696)()
/usr/local/src/sage/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/functions/trig.pyc in _evalf_(self, x, parent)
    732         if parent is float:
    733             return math.acos(1/x)
--> 734         return (1/x).arccos()
    735
    736     def _eval_numpy_(self, x):
AttributeError: 'complex' object has no attribute 'arccos'

comment:28 Changed 6 years ago by ppurka

  • Cc vbraun added

What do you make of this? Bug in cython? cc: vbraun

sage: from sage.plot.misc import setup_for_eval_on_grid
sage: f, d = setup_for_eval_on_grid(arcsec, [(-2, 2)])
sage: f(-0.992725195401)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-a06f3eb7afeb> in <module>()
----> 1 f(-RealNumber('0.992725195401'))

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/ext/interpreters/wrapper_rdf.so in sage.ext.interpreters.wrapper_rdf.Wrapper_rdf.__call__ (sage/ext/interpreters/wrapper_rdf.c:1689)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/function.so in sage.symbolic.function.BuiltinFunction.__call__ (sage/symbolic/function.cpp:9088)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__complex__ (sage/symbolic/expression.cpp:8174)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._eval_self (sage/symbolic/expression.cpp:6973)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/functions/trig.pyc in _evalf_(self, x, parent, algorithm)
    732         if parent is float:
    733             return math.acos(1/x)
--> 734         return (1/x).arccos()
    735
    736     def _eval_numpy_(self, x):

AttributeError: 'complex' object has no attribute 'arccos'
sage:
Exiting Sage (CPU time 0m0.25s, Wall time 0m12.78s).


basu@plantain:~/Installations/sage [test-float] $ git diff develop |cat  # insert a print statement
diff --git a/src/sage/symbolic/function.pyx b/src/sage/symbolic/function.pyx
index eed56e6..75c723d 100644
--- a/src/sage/symbolic/function.pyx
+++ b/src/sage/symbolic/function.pyx
@@ -934,6 +934,7 @@ cdef class BuiltinFunction(Function):
             # conversion to the original parent failed
             # we try if it works with the corresponding complex domain
             if org_parent is float:
+                print "DEBUG: it is float - {}".format(float(res))
                 try:
                     return complex(res)
                 except (TypeError, ValueError):



basu@plantain:~/Installations/sage [test-float] $ ./sage -b >& /dev/null ; ./sage
┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 6.3.beta2, Release Date: 2014-05-24                   │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: from sage.plot.misc import setup_for_eval_on_grid
sage: f, d = setup_for_eval_on_grid(arcsec, [(-2, 2)])
sage: f(-0.992725195401)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-a06f3eb7afeb> in <module>()
----> 1 f(-RealNumber('0.992725195401'))

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/ext/interpreters/wrapper_rdf.so in sage.ext.interpreters.wrapper_rdf.Wrapper_rdf.__call__ (sage/ext/interpreters/wrapper_rdf.c:1689)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/function.so in sage.symbolic.function.BuiltinFunction.__call__ (sage/symbolic/function.cpp:9099)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__float__ (sage/symbolic/expression.cpp:8022)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._eval_self (sage/symbolic/expression.cpp:6973)()

/home/basu/Installations/sage/local/lib/python2.7/site-packages/sage/functions/trig.pyc in _evalf_(self, x, parent, algorithm)
    731         """
    732         if parent is float:
--> 733             return math.acos(1/x)
    734         return (1/x).arccos()
    735

ValueError: math domain error
sage:

The error changes to ValueError from AttributeError. How can a print statement change the call sequence?

comment:29 Changed 6 years ago by ppurka

  • Branch set to u/ppurka/automatic_exclusion_of_non_domain_points_in_things_like_arcsec

comment:30 Changed 6 years ago by ppurka

  • Commit set to c31f63ed3860dbf2368ed17b3f54d8dd0ebf0331

I pushed the rebased branch (only whitespace errors apparently). The error mentioned about complex numbers is not introduced by the plot code.


New commits:

4193ca4This patch fixes two things, both related to exclusion of invalid plot points.
c31f63efix parametric plots that fail due to invalid plot points

comment:31 Changed 6 years ago by kcrisman

Thanks for this digging, ppurka. I have to say there isn't a lot in that changelog that seems likely. Maybe #13933? But see below.


This ticket has exposed some bugs in trig.py as well.

sage: arcsec(-0.992725195401)
NaN
sage: arcsec(float(-0.992725195401))
<snip>
    732         if parent is float:
    733             return math.acos(1/x)
--> 734         return (1/x).arccos()
    735 
    736     def _eval_numpy_(self, x):

AttributeError: 'complex' object has no attribute 'arccos'

Compare to behavior in Sage 5.2:

sage: arcsec(-0.992725195401)
arcsec(-0.992725195401000)
sage: arcsec(float(-0.992725195401))
arcsec(-0.992725195401)
sage: 

Related, I think - none of these take things nicely.

sage: arccos(1+2*i)
arccos(2*I + 1)
sage: arccos(CC(1,2))
1.14371774040242 - 1.52857091948100*I
sage: arccos(CDF(1,2))
1.1437177404 - 1.52857091948*I
sage: arccos(complex(1,2))
<snip>
TypeError: Unable to convert x (='(1+2j)') to real number.

Anyway, I think I got it. Old behavior:

sage: arcsec(float(-0.992725195401))
arcsec(-0.992725195401)

This is because before #13933, arcsec.__call__, a BuiltinFunction (not GinacFunction) was inheriting its __call__ method directly from Function. Then we hit this branch of the code which turns the argument into a symbolic argument.

But now, arcsec (still a BuiltinFunction now) uses the __call__ method from what used to be the method for only GinacFunction, but which is now the method also for BuiltinFunction. So after doing the same thing it did before, getting the symbolic version

sage: a = float(-0.992725195401)
KeyboardInterrupt
sage: res = super(sage.symbolic.function.BuiltinFunction,arcsec).__call__(a,coerce=True)
sage: res
arcsec(-0.992725195401)

it now continues on instead of returning, and tries to evaluate res as a complex, which causes us to go back to _evalf_ of arcsec

sage: complex(res)
---------------------------------------------------------------------------
AttributeError: 'complex' object has no attribute 'arccos'

I'm not sure if the correct fix is to just make sure trig.py can handle this, or whether this is a possible bug in the way BuiltinFunction was changed. I'm going to ask at #13933.


Finally, from one of the original requesters, some more test cases.

Thanks for looking into this. The following page is a problem set in our online text that has six interacts, the last two with my workaround for handling arcsec(x) and arccsc(x), as well as their derivatives, which involve 1/sqrt(x2-1), another good test function, along with its reciprocal, sqrt(x2-1).

http://calculuscourse.maa.org/main2/Chapter5/Section5-5/Chapter5-5-6M.html

If you enter any of these in those last two "Graph" interacts (Problem 6 or 7), they will be drawn correctly -- because nothing is drawn between -1 and 1. [But arcsec(x/2) will cause an error message, and arcsec(2*x) will not be completely drawn.] If you enter any of these in the third or fourth interacts (Problems 4 and 5), there will either be an error message (arcsec or arccsc) or the graph will be incorrectly drawn between 1 and -1 (sqrt(x2-1) or 1/sqrt(x2-1)).

I think the general class of functions for which this particular problem exists is those for which the domain is interrupted by an interval of finite length greater than 0. The inverse trig functions seem to involve a different problem when trying to compute non-real values, but I don't understand the error messages.

comment:32 Changed 6 years ago by kcrisman

I've opened #16439 for the trig stuff in general. I'm not sure yet whether I want to open a new ticket for the current error, though I'm inclined to so that this can get in. (Though I haven't reviewed it yet!)

comment:33 follow-up: Changed 6 years ago by kcrisman

Also, does this change completely get rid of the whole warnings for so-and-so many unplotted points? I am wondering whether we might want to include that still anyway for instances where exclude wasn't set by the user - because sometimes people won't realize it. For instance, now presumably plotting log(x) from -1 to 1 won't raise a warning any more, but that would be a regression.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 6 years ago by ppurka

Replying to kcrisman:

Also, does this change completely get rid of the whole warnings for so-and-so many unplotted points? I am wondering whether we might want to include that still anyway for instances where exclude wasn't set by the user - because sometimes people won't realize it. For instance, now presumably plotting log(x) from -1 to 1 won't raise a warning any more, but that would be a regression.

Where do you see this behavior? I can not reproduce it. This is the output in sage-6.1.1

basu@plantain:~/Installations/sage [t/13246/automat] $ ~/Installations/sage-6.1.1.server/sage
┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 6.1.1, Release Date: 2014-02-04                       │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
sage: plot(log, -1, 1)
verbose 0 (2411: plot.py, generate_plot_points) WARNING: When plotting, failed to evaluate function at 100 points.
verbose 0 (2411: plot.py, generate_plot_points) Last error message: 'can't convert complex to float'

And this is the almost same (only change in line numbers) output after this patch

basu@plantain:~/Installations/sage [t/13246/automat] $ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ Sage Version 6.3.beta2, Release Date: 2014-05-24                   │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: plot(log(x), -1, 1)
verbose 0 (2467: plot.py, generate_plot_points) WARNING: When plotting, failed to evaluate function at 100 points.
verbose 0 (2467: plot.py, generate_plot_points) Last error message: 'can't convert complex to float'

comment:35 in reply to: ↑ 34 Changed 6 years ago by kcrisman

Where do you see this behavior? I can not reproduce it. This is the output in sage-6.1.1

Oh, great! (Sorry, I hadn't tried this branch yet because of not wanting to deal with some rebuild issues because the branch I'm on has a couple (s)pkg changes.)

But I suspect that anyway we don't have that warning any more unless it's at the endpoints of the domain that we have the problem, right? So I guess I'm just wondering whether, to provide the same service we did before, still printing a warning when there was exclusion of non-domain points (and providing an explicit example in the documentation of how to set the verbosity to not print the warning) would be useful. I'm inclined to say yes.

comment:36 Changed 6 years ago by ppurka

Ok. I can't test this now with arcsec all broken and all. But my doctest has a set_verbose(-1) before plotting the arcsec. I suspect that was done to suppress exactly the kind of information that you want to display - this was probably done to make the doctest cleaner.

comment:37 Changed 6 years ago by kcrisman

Fair enough.

Regarding arcsec - do we want to then have this rely on #16439, or should we just try to get this in anyway, perhaps with some of the other doctests suggested at the end of comment:31?

comment:38 Changed 6 years ago by ppurka

  • Dependencies set to #16439

In my opinion, we should wait on #16439 so that the doctests pass successfully.

comment:39 Changed 6 years ago by kcrisman

  • Milestone changed from sage-6.3 to sage-pending

We could make this sage-pending in that case.

comment:40 Changed 6 years ago by git

  • Commit changed from c31f63ed3860dbf2368ed17b3f54d8dd0ebf0331 to 148aafbc155f5fceb9f156a725067e1b9f5c5436

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

a18bf04Merge branch 'develop' into t/13246/automatic_exclusion_of_non_domain_points_in_things_like_arcsec
b34ee39allow some trigonometric functions to take complex as input
c4ba30dadd couple of doctests for complex input
dd2ac84Merge branch 't/16439/allow_inverse_trig_functions_to_all_take_complex_input' into t/13246/automatic_exclusion_of_non_domain_points_in_things_like_arcsec
148aafbadd more doctests for auto exclusion functionality

comment:41 Changed 6 years ago by ppurka

  • Milestone changed from sage-pending to sage-6.3
  • Status changed from needs_work to needs_review

Ok. I added some more doctests. We should review #16439 and get that in, and get this in.

There are three functions (in #16439) that still do not work, but probably no one uses them with complex input otherwise it wouldn't have remained buggy for a decade. I suggest we move them to another ticket.

comment:42 follow-up: Changed 5 years ago by rws

  • Reviewers set to Ralf Stephan

While the plot doctests pass, in interactive Sage I get

sage: plot(sqrt(x^2-1), -2, 2)
verbose 0 (2474: plot.py, generate_plot_points) WARNING: When plotting, failed to evaluate function at 100 points.
verbose 0 (2474: plot.py, generate_plot_points) Last error message: 'math domain error'

before the plot.

In the plot docs, "This reduces the possibility of, e.g., sampling sin only" please render sin via latex, or backticks.

comment:43 in reply to: ↑ 42 Changed 5 years ago by kcrisman

While the plot doctests pass, in interactive Sage I get

sage: plot(sqrt(x^2-1), -2, 2)
verbose 0 (2474: plot.py, generate_plot_points) WARNING: When plotting, failed to evaluate function at 100 points.
verbose 0 (2474: plot.py, generate_plot_points) Last error message: 'math domain error'

before the plot.

As discussed above, that is actually desired behavior.

Changed 5 years ago by kcrisman

comment:44 follow-ups: Changed 5 years ago by kcrisman

parametric_plot((x, arcsec(x)), (x, -2, 2))
<usual complex to float error>

from the doctest doesn't seem to work for me. I wonder if this worked before #16439?


I have a different comment.

sage: plot(sqrt(x^2-1), -2, 2, plot_points=500, ymin=0)

yields As you can see, it never quite makes it down to the horizontal axis. If you use the endpoints 1 and 2, it does; 0 and 2 doesn't. (And presumably didn't in the past, either.) Anyway, that is annoying, though I'm not sure there is much to do about it.

comment:45 Changed 5 years ago by kcrisman

  • Reviewers changed from Ralf Stephan to Ralf Stephan, Karl-Dieter Crisman

comment:46 in reply to: ↑ 44 ; follow-up: Changed 5 years ago by rws

Replying to kcrisman:

parametric_plot((x, arcsec(x)), (x, -2, 2))
<usual complex to float error>

from the doctest doesn't seem to work for me. I wonder if this worked before #16439?

It does work when you checkout this branch only. I didn't report it because I thought the version of the file in this ticket was more up-to-date.

comment:47 Changed 5 years ago by git

  • Commit changed from 148aafbc155f5fceb9f156a725067e1b9f5c5436 to a01095ea161f0adae68692c67174771232c80675

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

a01095efix latex according to reviewer comments

comment:48 Changed 5 years ago by ppurka

@kcrisman: The parametric plot works here. See http://i.imgur.com/YejcPZq.jpg

@rws: the doctests don't show the warnings because I set verbose(-1) before the plot.

comment:49 in reply to: ↑ 46 ; follow-up: Changed 5 years ago by kcrisman

from the doctest doesn't seem to work for me. I wonder if this worked before #16439?

It does work when you checkout this branch only. I didn't report it because I thought the version of the file in this ticket was more up-to-date.

That's odd, and I can confirm what you say. I clearly don't get the wonders of git yet. Sigh. (But what is different here from there with respect to complex/float?)

comment:50 in reply to: ↑ 49 ; follow-up: Changed 5 years ago by ppurka

Replying to kcrisman:

I clearly don't get the wonders of git yet. Sigh.

May I suggest my write up ;-)

comment:51 in reply to: ↑ 50 Changed 5 years ago by kcrisman

I clearly don't get the wonders of git yet. Sigh.

May I suggest my write up ;-)

Maybe you need to find a way to include some of this in the developer guide...

comment:52 in reply to: ↑ 44 Changed 5 years ago by kcrisman

As you can see, it never quite makes it down to the horizontal axis. If you use the endpoints 1 and 2, it does; 0 and 2 doesn't. (And presumably didn't in the past, either.) Anyway, that is annoying, though I'm not sure there is much to do about it.

Here is what is going on. For user-provided exclusion, we have

        if isinstance(exclude, (list, tuple)):
            exclude = sorted(exclude)
            # We make sure that points plot points close to the excluded points are computed

but we don't have something similar to that for the 'auto-excluded points'. I don't see an easy way to do this, though.

I am also annoyed (this has nothing to do with this patch) by the inconsistency between exclude is not None, not exclude, exclude != None, and so forth. There are subtle things that can happen here, though I think that in this case they don't.


Now for something that I still see as a bug, though probably not a problem for this ticket.

sage: polar_plot(sin(sqrt(x^2-1)), (x, 0, 2*pi), exclude=[2,3])
sage: polar_plot(sin(sqrt(x^2-1)), (x, 0, 2*pi), exclude=[1/2,2,3])

Compare these. The first one does just what you want. The second one does not - presumably because I'm excluding a point already not in the domain, but why does that cause a problem? The others should still be there. You could say it's user error, but then again

sage: plot(x,(x,0,1),exclude=[-1,1/2])

works as desired, so I don't think that is why, and it occurs with and without this change. Actually, presumably this is via parametric from polar.

sage: parametric_plot((sqrt(x^2-1),sqrt(x^2-1/2)),(x,0,5), exclude=[2,3])
sage: parametric_plot((sqrt(x^2-1),sqrt(x^2-1/2)),(x,0,5), exclude=[1,2,3])

Is this a new ticket, or a wontfix? I'm inclined to say this is a bug.

comment:53 Changed 5 years ago by ppurka

Oh darn. Touching this exclude this has opened up a full set of bugs. I have fixed the exclude is not != None kind of stuff but the parametric plot stuff is a different beast.

The problems with parametric plot appear mainly because of a disconnect between the actual data points on the graph and the exclude points -- the exclude points are not the actual data points (that appear on the plot) but are actually the values of the parametric variable.

comment:54 Changed 5 years ago by git

  • Commit changed from a01095ea161f0adae68692c67174771232c80675 to 5c7fb75e184b2e034d53acf1807c58e2165ac7d0

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

5c7fb75do not use exclude both as list and as None

comment:55 Changed 5 years ago by ppurka

I am not clear how to fix the parametric plot. I can look into it in detail only over the weekend.

comment:56 Changed 5 years ago by git

  • Commit changed from 5c7fb75e184b2e034d53acf1807c58e2165ac7d0 to b90cb76eb10225c28257081feafdd4bb3effbd40

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

b90cb76sort and reverse the excluded_points in one command

comment:57 Changed 5 years ago by git

  • Commit changed from b90cb76eb10225c28257081feafdd4bb3effbd40 to e79bdb8e73d8ebdca5cb6782ca1e4ae7cd17b96e

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

e79bdb8fix parametric and polar plots if exclude point is outside the domain

comment:58 Changed 5 years ago by ppurka

I think I have fixed the problem with the parametric and polar plots. Needs review. :)

comment:59 follow-up: Changed 5 years ago by kcrisman

  • Status changed from needs_review to positive_review

Thank you for all your very hard work on this!

comment:60 in reply to: ↑ 59 Changed 5 years ago by ppurka

Replying to kcrisman:

Thank you for all your very hard work on this!

Thanks for your extensive testing! You have uncovered many bugs in this ticket :-)

comment:61 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

I get

sage -t --long src/sage/plot/plot.py
**********************************************************************
File "src/sage/plot/plot.py", line 1276, in sage.plot.plot.?
Failed example:
    parametric_plot((x, arcsec(x)), (x, -2, 2))
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest sage.plot.plot.?[14]>", line 1, in <module>
        parametric_plot((x, arcsec(x)), (x, -Integer(2), Integer(2)))
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/misc/decorators.py", line 550, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/plot.py", line 1659, in parametric_plot
        return plot(funcs, *args, **kwargs)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/misc/decorators.py", line 705, in wrapper
        return func(*args, **kwds)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/misc/decorators.py", line 550, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/plot.py", line 1176, in plot
        G = _plot(funcs, *args, **kwds)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/plot.py", line 1383, in _plot
        newdata.append((fdata, g(x)))
      File "wrapper_rdf.pyx", line 80, in sage.ext.interpreters.wrapper_rdf.Wrapper_rdf.__call__ (build/cythonized/sage/ext/interpreters/wrapper_rdf.c:1689)
    TypeError: can't convert complex to float

comment:62 Changed 5 years ago by ppurka

This shouldn't happen at all. This is what I fixed in #16439. Does this happen if you apply this ticket after #16439?

comment:63 Changed 5 years ago by ppurka

hmm.. git is not merging the two tickets correctly. I will have to check.

comment:64 Changed 5 years ago by git

  • Commit changed from e79bdb8e73d8ebdca5cb6782ca1e4ae7cd17b96e to 384b5a23011b43380be2c434e2487121aaa7dea8

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

384b5a2catch TypeError after #16439 is applied

comment:65 Changed 5 years ago by ppurka

  • Status changed from needs_work to needs_review

@vbraun: this should fix the doctest error. This ticket by itself was working fine. Applying this after #16439 introduced a TypeError that I wasn't catching. Now it all works fine.

Edit: I think it is because of this git commit in #16439 (that is not merged into this ticket) that a TypeError started getting raised.

Last edited 5 years ago by ppurka (previous) (diff)

comment:66 Changed 5 years ago by ppurka

  • Status changed from needs_review to needs_work

breaks plot in this case:

sage: var('n,a')
sage: hbar, m = 1,1
sage: psi(x,t,n)=sqrt(2/a)*sin(n*pi*x/a)*e^(-i*n^2*pi^2*hbar*t/(2*m*a^2)); 
sage: print psi
sage: Psi(x,t)=1/sqrt(2)*psi(x,t,1)+1/sqrt(2)*psi(x,t,2); 
sage: print Psi
sage: P(x,t,a) = Psi.conjugate()*Psi
sage: print P.expand()
sage: plot(P(x,1,1),x,0,1)

comment:67 Changed 5 years ago by ppurka

  • Status changed from needs_work to needs_review

False alarm! It is working as intended. :-)

p = plot(P(x,1,1),x,0,1)
po = p._objects[0]
po.xdata
[0.0,0.5,1.0]

comment:68 Changed 5 years ago by kcrisman

I was happy with this, so I guess if Volker can test if the doctest error he saw is gone then we are okay.

comment:69 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:70 Changed 5 years ago by vbraun

  • Branch changed from u/ppurka/automatic_exclusion_of_non_domain_points_in_things_like_arcsec to 384b5a23011b43380be2c434e2487121aaa7dea8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.