Opened 8 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 8 years ago by
- Cc dsm added
comment:2 Changed 8 years ago by
- 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 8 years ago by
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 8 years ago by
- Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.
comment:5 Changed 8 years ago by
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: ↓ 7 Changed 8 years ago by
- 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: ↓ 8 Changed 8 years ago by
- 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 8 years ago by
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: ↓ 11 Changed 7 years ago by
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
- 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 3 years ago by
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
- Keywords beginner removed
I'm a beginner, and found this too hard!
comment:13 Changed 2 years ago by
- 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.
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.