Opened 9 years ago

Last modified 2 years ago

#10980 new defect

Make sure symbolic gridline values are okay

Reported by: kcrisman Owned by: jason, was
Priority: minor Milestone: sage-8.1
Component: graphics Keywords: beginner
Cc: jason, dsm Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Change History (13)

comment:1 Changed 9 years ago by kcrisman

  • Cc dsm added

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.

comment:2 Changed 9 years ago by kcrisman

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

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'.

comment:3 Changed 9 years ago by kcrisman

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.

comment:4 Changed 9 years ago by jason

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

comment:5 Changed 9 years ago by mdboom

Committed to matplotlib master here:

https://github.com/matplotlib/matplotlib/commit/0a9e86a91d9cb180919b99363cea1839a8f6196c

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

comment:6 follow-up: Changed 9 years ago by dsm

  • Report Upstream changed from Reported upstream. Little or no feedback. to N/A

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)?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by kcrisman

  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.

Replying to 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.

comment:8 in reply to: ↑ 7 Changed 9 years ago by dsm

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. :-)

comment:9 follow-up: Changed 7 years ago by 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).

comment:10 Changed 7 years ago by kcrisman

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

comment:11 in reply to: ↑ 9 Changed 4 years ago by jhonrubia6

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

comment:12 Changed 2 years ago by cturo

  • Keywords beginner removed

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

Last edited 2 years ago by cturo (previous) (diff)

comment:13 Changed 2 years ago by tscrim

  • Keywords beginner added
  • Milestone set to sage-8.1
  • Report Upstream changed from Fixed upstream, in a later stable release. to N/A

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

Note: See TracTickets for help on using tickets.