Opened 10 years ago

Closed 8 years ago

#13576 closed enhancement (fixed)

add options about custom markers to point2d

Reported by: Punarbasu Purkayastha Owned by: jason, was
Priority: minor Milestone: sage-6.4
Component: graphics Keywords:
Cc: Jason Grout, Eviatar Bach Merged in:
Authors: Punarbasu Purkayastha Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: d7d7809 (Commits, GitHub, GitLab) Commit: d7d7809e5af59a8de32ac6e806dc93f5e91ecc12
Dependencies: Stopgaps:

Status badges

Description (last modified by Punarbasu Purkayastha)

point2d by default doesn't allow customization of markers. The patch below adds this functionality by adding two extra options marker and markeredgecolor.


Apply the git branch.

Attachments (2)

trac_13576-list_plot_doc.patch (2.2 KB) - added by Punarbasu Purkayastha 10 years ago.
Apply to devel/sage
trac_13576-add_options_to_points.patch (5.9 KB) - added by Punarbasu Purkayastha 10 years ago.
Apply to devel/sage

Download all attachments as: .zip

Change History (41)

Changed 10 years ago by Punarbasu Purkayastha

Apply to devel/sage

comment:1 Changed 10 years ago by Punarbasu Purkayastha

Authors: Punarbasu Purkayastha
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Status: needs_reviewneeds_work
Work issues: add customization options to point

comment:3 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Summary: add documentation to list_plot about custom markersadd options about custom markers to point2d
Work issues: add customization options to point

Changed 10 years ago by Punarbasu Purkayastha

Apply to devel/sage

comment:4 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Status: needs_workneeds_review

comment:5 Changed 10 years ago by Karl-Dieter Crisman

Oh, ppurka, I would love you if this works, I always get stuck using line(... linestyle='') for this very thing!

Is there an option for the width of the marker edge? They kind of overwhelm the marker color with the default point size; I have to go to size=60 or so to have it really seem like an edge.

Am I correct in my suspicion that markeredgecolor sort of automatically implies faceted=True in mpl internally? At least, the plots look like it, and faceted=False does nothing when I use markeredgecolor. The markeredgecolor seems to govern the color of the facet; I assume this is right, but the quite different names might make one think they control two different things.

In general I really like this patch, though, including the careful small local improvements that have nothing to do with the ticket but which are still valuable. Passes tests, so... answer my questions and you might get a positive review. Thanks for looking into this!

comment:6 Changed 10 years ago by Punarbasu Purkayastha

I will need some time to recall what this patch is doing, and what other changes I had in mind regarding this.

I do remember that I resisted adding some extra keywords which are synonymous to the keywords in plot, list_plot, etc, and which have the same functionality as other keywords in this function. I think size is the same as markersize, color keyword is missing, markeredgecolor and faceted control the same thing, except the second is a boolean, and so on. Essentially, a bunch of inconsistencies on the Sage interface side mainly because the keywords used are the same as the mpl scatter command. It is really jarring that many different plot commands have different keywords for achieving the same effect.

Do you think it will be useful to either deprecate some keywords or add other keywords like color, markersize, etc which are similar to the keywords used in plot, list_plot, line, etc?

comment:7 in reply to:  6 Changed 10 years ago by Karl-Dieter Crisman

Cc: Jason Grout added
Reviewers: Karl-Dieter Crisman

Do you think it will be useful to either deprecate some keywords or add other keywords like color, markersize, etc which are similar to the keywords used in plot, list_plot, line, etc?

Maybe. I wonder whether Jason might have some comments.

comment:8 Changed 10 years ago by Jason Grout

I actually just added marker options to points the other day in my personal copy of Sage so I could do some plots that I wanted to do :).

I think someone ought to go through each plotting function and make a comprehensive sweep to clean up options and make things more consistent. Right now, it's very confusing.

comment:9 Changed 10 years ago by Punarbasu Purkayastha

Ok then, let's defer the cleanup to a different ticket - #13828 to be precise.

comment:10 in reply to:  5 Changed 10 years ago by Punarbasu Purkayastha

Replying to kcrisman:

Oh, ppurka, I would love you if this works, I always get stuck using line(... linestyle='') for this very thing!

Actually, it is true that line(... linestyle='') is more powerful than point (mpl's scatter). Many options are not present in the scatter command.

Is there an option for the width of the marker edge? They kind of overwhelm the marker color with the default point size; I have to go to size=60 or so to have it really seem like an edge.

There is no such option for scatter/point. But there is the option markeredgewidth for plot/line.

comment:11 Changed 10 years ago by Karl-Dieter Crisman

What I am hearing here is that there is no point in using mol's scatter instead of whatever we use for line2d. What does scatter have (speed, flexibility, something else) that mpl lines.Iine2D doesn't have?

comment:12 in reply to:  11 Changed 10 years ago by Punarbasu Purkayastha

Replying to kcrisman:

What I am hearing here is that there is no point in using mol's scatter instead of whatever we use for line2d. What does scatter have (speed, flexibility, something else) that mpl lines.Iine2D doesn't have?

Right. This could be best answered by mpl. Other than the cmap argument, which we don't use in point2d, I don't see how scatter is better than line2d.

Also, #13830 is relevant for any cleanup.

comment:13 Changed 9 years ago by Karl-Dieter Crisman

Well, let's defer that to another ticket as well - I was annoyed by this again last week.

Just one more question, about the plot3d method:

raise NotImplementedError("3D points can not be faceted.")

but you don't do the same thing for the marker option. It just silently fails. In fact, both of these work.

sage: point([(0,1),(1,2)],marker='^',size=100,markeredgecolor='purple',color='yellow').plot3d()
sage: point([(0,1,0),(1,2,1)],marker='^',size=100,markeredgecolor='purple',color='yellow')

But hopefully we are okay otherwise, I don't see any reason not to merge this other than perhaps wanting to document or raise an error here. Maybe to point people to a list of markers elsewhere in the documentation.

comment:14 Changed 9 years ago by Karl-Dieter Crisman

See also #13051.

comment:15 Changed 9 years ago by Eviatar Bach

Cc: Eviatar Bach added

I think it makes sense to have the default faceted colour be blue, in accordance with the default for lines.

comment:16 in reply to:  15 Changed 9 years ago by Punarbasu Purkayastha

Replying to eviatarbach:

I think it makes sense to have the default faceted colour be blue, in accordance with the default for lines.

I was about to make this change. Then I realized that the default marker color is blue. So, blue face with blue edge will not be visibly faceted at all, whereas blue on black can be just about discerned. So, if you change the default marker color, then change the edge color too to be consistent with your plot.

comment:17 in reply to:  13 Changed 9 years ago by Punarbasu Purkayastha

Replying to kcrisman:

but you don't do the same thing for the marker option. It just silently fails. In fact, both of these work.

sage: point([(0,1),(1,2)],marker='^',size=100,markeredgecolor='purple',color='yellow').plot3d()
sage: point([(0,1,0),(1,2,1)],marker='^',size=100,markeredgecolor='purple',color='yellow')

I can fix the first one, but not the second one. A proper fix for the second one requires extensive changes to sage/plot/plot3d/* to make sure that all invalid options raise errors. Currently, all invalid options are silently ignored. For instance,

sage: point((1,2,3), abcd=True)

will happily show you the plot.

comment:18 Changed 9 years ago by Punarbasu Purkayastha

In fact, I can't even fix the first one. It breaks about 24 doctests, which were all working earlier. I suggest we maintain the status quo for now.

comment:19 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:20 Changed 9 years ago by Punarbasu Purkayastha

I am quite disappointed that this was never merged. This came up yet again recently with a colleague of mine. Some of these things which should "just work" don't just work in Sage.

comment:21 Changed 9 years ago by Karl-Dieter Crisman

I think that I just lost track of what needed to be done, and even that I was the one more or less reviewing this. It sounds like you need to open a new ticket for my question, since it can't be fixed easily, and I'm a little confused about the last sentence in your response to Eviatar. Is that something that should be added with an example in documentation? I can't parse it right now for some reason - probably due to going to too many math talks this morning before 9 AM.

I totally agree this should be in, however.

comment:22 Changed 9 years ago by Punarbasu Purkayastha

  1. The plots in comment:13 point([(0,1,0),(1,2,1)],marker='^') and point([(0,1),(1,2)],marker='^').plot3d() like unsupported command should be fixed in #15002.
  1. @eviatarbach in comment:15 suggested that the edgecolor should be blue since the lines in our plots are blue by default. But, the default marker color is blue and a blue default edgecolor will simply make the edge "invisible". This is equivalent to just plotting a bigger marker. The current default of black edgecolor makes the black ring around the blue marker just about visible. This doesn't need any example. I am just saying that we should maintain status quo.

EDIT: Actually, the current default is *no* edgecolor. So, nothing is being changed here.

Last edited 9 years ago by Punarbasu Purkayastha (previous) (diff)

comment:23 Changed 9 years ago by Punarbasu Purkayastha

Branch: u/ppurka/ticket/13576
Created: Oct 7, 2012, 5:29:50 AMOct 7, 2012, 5:29:50 AM
Modified: Jan 18, 2014, 6:14:10 PMJan 18, 2014, 6:14:10 PM

comment:24 Changed 9 years ago by Punarbasu Purkayastha

Commit: 9305db7241e5fc666e8de794755e3ac2d81706b5
Description: modified (diff)

New commits:

9305db7add options marker and markeredgecolor to point2d

comment:25 Changed 9 years ago by Karl-Dieter Crisman

At the scatter plot doc I note that they have a list of markers. A (possibly different?) list is in the line doc. Maybe this should have that too? Of course, that is very closely related to #8570 but it would be a copy-and-paste thing here (assuming no mpl options had been added recently.)

I have to say that having both scatter plot and this is quite redundant, so if you ended up having a patch - sorry, commit - for #13830 that extended this I might be motivated to review that, assuming I finish reviewing this soon. (Looks good again, but now I want to test out using various colors and whatnot and I need to finish some other things first.)

comment:26 Changed 9 years ago by Karl-Dieter Crisman

Dumb comment - I know the defaults would mean this could only happen if someone purposely used the graphics primitive. But

scatteroptions['marker'] = options.pop('marker')

maybe should have the optional default argument.

comment:27 in reply to:  26 ; Changed 9 years ago by Punarbasu Purkayastha

Status: needs_reviewneeds_work
Work issues: marker, check list of markers

Replying to kcrisman:

Dumb comment - I know the defaults would mean this could only happen if someone purposely used the graphics primitive. But

scatteroptions['marker'] = options.pop('marker')

maybe should have the optional default argument.

marker should always be defined because Point class is never exposed to the user and the method for points has @options(marker='o').

The list of markers might be different between the two. We should probably just point people to the other list; for example the list in plot. The list should probably be compared with mpl and checked to make sure that it is up to date.

comment:28 in reply to:  27 Changed 9 years ago by Karl-Dieter Crisman

Dumb comment - I know the defaults would mean this could only happen if someone purposely used the graphics primitive. But

scatteroptions['marker'] = options.pop('marker')

maybe should have the optional default argument.

marker should always be defined because Point class is never exposed to the user and the method for points has @options(marker='o').

Yes, but in principle someone could use sage.plot.graphics.point.Point or whatever... okay, I give in :) since we usually don't do this.

The list of markers might be different between the two. We should probably just point people to the other list; for example the list in plot. The list should probably be compared with mpl and checked to make sure that it is up to date.

Yes, if you can do that and update your branch that would be great. I won't get time tonight to do the real testing but I really don't anticipate anything and I do promise I will do it this time!

comment:29 Changed 9 years ago by git

Commit: 9305db7241e5fc666e8de794755e3ac2d81706b57542143d0f836d3046a34544fb09d3b22fb204aa

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

7542143add additional marker information to plot()

comment:30 Changed 9 years ago by Punarbasu Purkayastha

Status: needs_workneeds_review
Work issues: marker, check list of markers
  1. The information about marker was already present in points. The documentation asks users to look at points.option to find the information about allowed options. points.option does mention marker and was already asking the reader to look at plot for more information.
  1. The marker list in plot was mostly complete. Added a few more.

comment:31 Changed 9 years ago by git

Commit: 7542143d0f836d3046a34544fb09d3b22fb204aa1493a781555cccdb48f8037e7aba802bc8c64cb4

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

1493a78fix documentation for scatter_plot and points

comment:32 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:33 Changed 8 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:34 Changed 8 years ago by Ralf Stephan

Status: needs_reviewneeds_work
Work issues: rebase

comment:35 Changed 8 years ago by git

Commit: 1493a781555cccdb48f8037e7aba802bc8c64cb4d7d7809e5af59a8de32ac6e806dc93f5e91ecc12

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

d7d7809merge develop (sage-6.2) on to current ticket

comment:36 Changed 8 years ago by Punarbasu Purkayastha

Status: needs_workneeds_review
Work issues: rebase

Thanks for checking the ticket. The changes from develop have been merged on to current ticket.

comment:37 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:38 Changed 8 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_review

Eventually we'll need to unify scatter plot and point, I think there is even a ticket for this...

I would love to add more examples sometime.

sage: plot(x^2,marker="$x^2$",markeredgecolor='red')

Hilarious!

Anyway, I'm happy with this.

comment:39 Changed 8 years ago by Volker Braun

Branch: u/ppurka/ticket/13576d7d7809e5af59a8de32ac6e806dc93f5e91ecc12
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.