Make sure symbolic gridline values are okay

[kcrisman]

From this ask.sagemath.org question.

plot(x,0,2,gridlines=([sqrt(2)],[]))

blows up.

The fix is probably to make sure that symbolic input with a n() method passes something right to matplotlib, which of course cannot handle sqrt(2). Possibly beginner ticket.

Note: A beginner who is interested in working on this ticket can skip down to comment:13.

CC: @jasongrout @sagetrac-dsm

Component: graphics

Author: Atte Niemi

Branch/Commit: 702d272

Reviewer: Dave Morris

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

[kcrisman]

comment:1

Update: perhaps this is something that could be considered an upstream bug - or a bug that we give a len() to symbolic expressions, see DSM's answer in the question.

[kcrisman]

comment:2

Update: From the question, Jason says

I think matplotlib should check for iterability by either: calling iter(obj) and checking for a TypeError, or doing isinstance(obj, collections.Iterable). 

Changing to 'not yet reported upstream'.

[kcrisman]

Upstream: Not yet reported upstream; Will do shortly.

[kcrisman]

comment:3

We'll still have to include a fix, of course; an mpl change will just make sure the error is caught earlier by them, but we should be making sure we turn sqrt(2) into something plottable.

[jasongrout]

comment:4

It is reported: http://thread.gmane.org/gmane.comp.python.matplotlib.devel/10009

[jasongrout]

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

[sagetrac-mdboom]

comment:5

Committed to matplotlib master here:

matplotlib/matplotlib@0a9e86a

Since this has a likelihood of accidental breakage, this is not on the bugfix branch.

[sagetrac-dsm]

Changed upstream from Reported upstream. Little or no feedback. to none

[sagetrac-dsm]

comment:6

So what now? This bug had three causes: (1) we weren't passing floats, (2) symbolic expressions have a __len__ even though they probably shouldn't, and (3) matplotlib used a suboptimal way of determining iterability. If any of those three didn't fire then there's no bug..

(3) is fixed upstream and we'll eventually pick it up, which leaves (1) and (2). Should we fix (1) here and open a new ticket to fix (2)?

[kcrisman]

Upstream: Fixed upstream, but not in a stable release.

[kcrisman]

comment:7

Replying to @sagetrac-dsm:

So what now? This bug had three causes: (1) we weren't passing floats, (2) symbolic expressions have a __len__ even though they probably shouldn't, and (3) matplotlib used a suboptimal way of determining iterability. If any of those three didn't fire then there's no bug..

(3) is fixed upstream and we'll eventually pick it up, which leaves (1) and (2). Should we fix (1) here and open a new ticket to fix (2)?

Yes, we fix (1) here. I don't think that (2) necessarily has to be fixed, as long as we're aware of it. It's presumably been around a while, and we'd have to deprecate it. Though of course number_of_operands is more accurate.

sage: f(x)=x^2+1+sin(ln(x))
sage: len(f)
3

seems reasonable...

Changing the upstream report. NA is only if literally NA. In this case, there were a couple things going on, so not NA; according to the post above, it's fixed but not yet in the bugfix branch i.e. stable release.

[sagetrac-dsm]

comment:8

Replying to @kcrisman:

Changing the upstream report. NA is only if literally NA. In this case, there were a couple things going on, so not NA; according to the post above, it's fixed but not yet in the bugfix branch i.e. stable release.

Oops! That wasn't intended. I couldn't decide if it counted as a stable or unstable release, and I guess I set it to NA instead of restoring it. [I find it surprisingly easy to make errors on trac. I think I've assigned a couple of tickets to myself now..]

As for the len issue, I'm inclined to agree with Jason that it should go. If there's no interpretation of iter(sqrt(x)) natural enough to be canonical, then IMHO len(sqrt(2)) shouldn't work either. OTOH if we think that it's natural that len(sqrt(2)) should be two, because len counts the operands, then list(sqrt(2)) should be list((sqrt(2)).operands()) and give [2, 1/2]. That goes against my instincts, because I'd assume that taking the len of an expression, or iterating over it, would work over the "terms" of an expression. But len(x) == 0, because there are no operations so there are no operands, which is completely consistent but isn't what I'd have expected.

I don't think it's important enough for me to learn how Sage deprecation policies work, though, given that I'm still having trouble counting colons. :-)

[kcrisman]

comment:9

This does actually work now, by the way.

plot(x,0,2,gridlines=([sqrt(2)],[]))

This doesn't resolve the issue of whether symbolic things should have a len.

    def __len__(self):
        """
        Returns the number of arguments of this expression.

        EXAMPLES::
        
            sage: var('a,b,c,x,y')
            (a, b, c, x, y)
            sage: len(a)
            0
            sage: len((a^2 + b^2 + (x+y)^2))
            3
            sage: len((a^2))
            2
            sage: len(a*b^2*c)
            3
        """
        return self.number_of_operands()

But this has been in for at least three years (the Pynac switch).

[kcrisman]

Changed upstream from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

[sagetrac-jhonrubia6]

comment:11

Replying to @kcrisman:

This does actually work now, by the way.

plot(x,0,2,gridlines=([sqrt(2)],[]))

This doesn't resolve the issue of whether symbolic things should have a len.

    def __len__(self):
        """
        Returns the number of arguments of this expression.

        EXAMPLES::
        
            sage: var('a,b,c,x,y')
            (a, b, c, x, y)
            sage: len(a)
            0
            sage: len((a^2 + b^2 + (x+y)^2))
            3
            sage: len((a^2))
            2
            sage: len(a*b^2*c)
            3
        """
        return self.number_of_operands()

But this has been in for at least three years (the Pynac switch).

so, should this go to a closed state? or instead go to a need_work state? in my opinion it should move to a sage-devel discussion before

[sagetrac-cturo]

comment:12

I'm a beginner, and found this too hard!

[tscrim]

comment:13

This is fixed now, so it just needs a doctest added.

[tscrim]

Changed upstream from Fixed upstream, in a later stable release. to none

[DaveWitteMorris]

comment:14

To close this ticket, we just need a doctest that checks whether the example in the description gives good output, instead of an error.

The discussion of whether symbolic expressions should have a length has been moved to #29738.

[hur]

Branch: u/gh-hur/make_sure_symbolic_gridline_values_are_okay

[DaveWitteMorris]

Commit: ac6f6a7

[DaveWitteMorris]

comment:17

Thanks for fixing this.

Please add your (real) name in the Authors: field and set the status of the ticket to needs review. (To do both of these, click on the Modify Ticket button to open up a panel that lets you make these changes. Then click Submit changes when you are done.) This tells potential reviewers and the automated patchbots that you have completed your work.

---

New commits:

ac6f6a7

Add doctest for #10980

[fchapoton]

comment:18

This line

Verify that :trac `10980` is fixed

should be

Verify that :trac:`10980` is fixed::

[sagetrac-git]

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

788e812

Correct errors in doctest for #10980

[sagetrac-git]

Changed commit from ac6f6a7 to 788e812

[hur]

Author: Atte Niemi

[DaveWitteMorris]

comment:21

The correction is not right, because the second colon needs to come immediately after "trac", not at the end:

:trac:`10980`

not

:trac`10980`:

It needs to be done this way so that sphinx will make a correct hyperlink in the sagemath documentation.

[mkoeppe]

comment:23

Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage

[mkoeppe]

comment:24

Setting a new milestone for this ticket based on a cursory review.

[DaveWitteMorris]

Reviewer: Dave Morris

[DaveWitteMorris]

Changed branch from u/gh-hur/make_sure_symbolic_gridline_values_are_okay to public/10980

[vbraun]

Changed commit from 788e812 to 702d272

[vbraun]

Changed branch from public/10980 to 702d272