Ticket #13246 (needs_review defect)

Opened 11 months ago

Last modified 10 months ago

Automatic exclusion of non-domain points in things like arcsec

Reported by: kcrisman Owned by: jason, was
Priority: major Milestone: sage-5.10
Component: graphics Keywords:
Cc: ddrake, ppurka Work issues:
Report Upstream: N/A Reviewers:
Authors: Punarbasu Purkayastha Merged in:
Dependencies: Stopgaps:

Description (last modified by ppurka) (diff)

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 trac_13246-automatic-exclusion.patch Download to devel/sage

Attachments

tmp_1.png Download (21.1 KB) - added by kcrisman 11 months ago.
arcsec plot
trac_13246-automatic-exclusion.patch Download (4.2 KB) - added by ppurka 10 months ago.
apply to devel/sage

Change History

Changed 11 months ago by kcrisman

arcsec plot

comment:1 Changed 11 months 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: ↓ 3 Changed 11 months 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 11 months 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 11 months 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 11 months 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 11 months ago by ppurka (previous) (diff)

comment:6 Changed 11 months 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 10 months ago by ppurka

Can you check many different plots with and without this patch Download? 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 10 months 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 10 months ago by ppurka (previous) (diff)

comment:9 Changed 10 months 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 10 months 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 10 months 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 10 months ago by ppurka

  • Description modified (diff)

comment:13 Changed 10 months ago by jdemeyer

Please fill in your real name as Author.

comment:14 Changed 10 months ago by ppurka

  • Authors set to Punarbasu Purkayastha

comment:15 Changed 10 months ago by ppurka

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

Changed 10 months ago by ppurka

apply to devel/sage

comment:16 Changed 10 months ago by ppurka

  • Status changed from needs_work to needs_review
  • Work issues fix doctests deleted
Note: See TracTickets for help on using tickets.