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

plot in piecewise regression #21618

Open
kcrisman opened this issue Oct 1, 2016 · 14 comments
Open

plot in piecewise regression #21618

kcrisman opened this issue Oct 1, 2016 · 14 comments

Comments

@kcrisman
Copy link
Member

kcrisman commented Oct 1, 2016

P = piecewise([((0,2),x),((2,6),2)])
print P
plot(P)

Before this would have plotted the whole thing, now it defaults to the usual plotting only from -1 to 1 (which of course gives an error).

Moreover pieces are joined and detect_poles=True option skips the origin for no good reason:

P = piecewise([((-1,2),x),((2,5),1)])
print P
plot(P, -1, 5, detect_poles=True)

CC: @paulmasson

Component: symbolics

Branch/Commit: u/rws/21618-1 @ c008c29

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

@kcrisman kcrisman added this to the sage-7.4 milestone Oct 1, 2016
@novoselt

This comment has been minimized.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 3, 2016

comment:2

The old Piecewise has a custom plot method. This method is omitted in the current piecewise.

@paulmasson paulmasson mannequin changed the title plot in piecewise regression (?) plot in piecewise regression Oct 3, 2016
@kcrisman
Copy link
Member Author

kcrisman commented Oct 4, 2016

comment:3

Correct. So it would be nice to emulate that so everyone's code doesn't break - I had a not-so-nice time drawing graphs on the fly in class today because of this. It seems reasonable, since piecewise is an unusual situation.

@rwst
Copy link

rwst commented Oct 5, 2016

Branch: u/rws/plot_in_piecewise_regression

@rwst
Copy link

rwst commented Oct 5, 2016

comment:5

Oops I just saw the second complaint. This will not be as easy.


New commits:

c98505b21618: plotting piecewise without need of giving domain interval

@rwst
Copy link

rwst commented Oct 5, 2016

Commit: c98505b

@rwst
Copy link

rwst commented Oct 5, 2016

comment:7

Note that your command plot(P, -1, 5, detect_poles=True) also does not work on Sage-7.1, i.e., the old piecewise, and plot(P, detect_poles=True) also has a gap at x=0. So, this particularly is rather an enhancement of plot, not a bug in piecewise.

@rwst
Copy link

rwst commented Oct 6, 2016

Changed branch from u/rws/plot_in_piecewise_regression to u/rws/21618-1

@rwst
Copy link

rwst commented Oct 6, 2016

New commits:

c008c2921618: plotting piecewise without need of giving domain interval

@rwst
Copy link

rwst commented Oct 6, 2016

Changed commit from c98505b to c008c29

@novoselt
Copy link
Member

comment:10
  1. Why there is commented code? Reference to some ticket explaining stuff is fine, but not strange imports etc.
  2. Why there is extra printing of pieces???
  3. If I do specify the domain, then each piece is plotted over this domain!

I'll see if I can resolve all this in the next hour or so.

@novoselt
Copy link
Member

comment:11

Address 1&2 and simplify a bit to get

        def plot(cls, self, parameters, variable, *args, **kwds):
            from sage.plot.all import plot, Graphics
            g = Graphics()
            for (dom, fun) in self.items():
                g += plot(fun, (variable, dom.inf(), dom.sup()), *args, **kwds)
                # If it's the first piece, pass all arguments. Otherwise,
                # filter out 'legend_label' so that we don't add each
                # piece to the legend separately (trac #12651).
                kwds.pop('legend_label', None)
            return g

3 is tricky, however, because of a variety of ways to pass left and right bounds. I think we should factor out the logic of getting these bounds into a separate function which will say always set xmin/xmax keywords, then we can call this function in the top level plot as well as plot methods and have a guaranteed uniform behavior. Thoughts?

There is also
4. No documentation and examples at all!!!

@rwst
Copy link

rwst commented Oct 11, 2016

comment:12

Andrey, please go ahead with your own branch, the plotting code is quite opaque and uninteresting to me, and I'm into other projects right now.

@kcrisman
Copy link
Member Author

When fixing this (if ever), we should check whether #11225 is also resolved at the same time (which is likely).

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