Opened 7 years ago

Closed 7 years ago

#14640 closed defect (fixed)

Refactor the plot_expose function into a method

Reported by: vbraun Owned by: jason, was
Priority: major Milestone: sage-5.11
Component: graphics Keywords:
Cc: nthiery, tscrim Merged in: sage-5.11.beta0
Authors: Volker Braun Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

This is just a needless slap in the face of OOP ;-)

Also, a better name should be picked. How about plot().describe()

Finally, sort by zorder and then alphabetically for doctesting sanity.

Attachments (2)

trac_14640_refactor_plot_expose.patch (12.7 KB) - added by vbraun 7 years ago.
Initial patch
trac_14640-refactor_plot_expose-review-ts.patch (5.8 KB) - added by tscrim 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:2 Changed 7 years ago by vbraun

  • Description modified (diff)

Changed 7 years ago by vbraun

Initial patch

comment:3 Changed 7 years ago by vbraun

  • Cc nthiery tscrim added
  • Status changed from new to needs_review

comment:4 Changed 7 years ago by tscrim

The patch looks good to me, but how about a more specific (descriptive :P) name such as text_description()?

Best,
Travis

comment:5 Changed 7 years ago by vbraun

Active is better than the passive verb from. And of course its text, did you expect that it is going to read it out to your through the speaker? ;-)

comment:6 Changed 7 years ago by tscrim

  • Authors set to Volker Braun
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

That would be awesome if it did. I'm just thinking when I see this on tab completion that it is somewhat vague (but that's what the doc is for). Anyhow, I can't think of a better name, so I'm setting this to positive review. Nicolas, if you have any issues with the patch, feel free to set this back.

comment:7 Changed 7 years ago by nthiery

Hi Volker,

+1 for the change: I was just lazy at this point touching yet another thing outside of root systems and risking to create a conflict; and also launching a discussion about what would be the right output, etc. Thanks for fixing my lazyness!

As for the name, why is active better? Isn't the convention to use a noun describing the result for methods whose main purpose is to return something, and a verb describing the action for methods whose main purpose is to change self? At this point I would lean for "description".

Oh, and we might want to check if e.g. matplotlib does not have already a convention for this. Related things are the x3d_str and mtl_str methods in 3d plots.

Anyway, just throwing ideas in the air. Please proceed as you see fit!

comment:8 Changed 7 years ago by vbraun

I'd say noun if you return an object, otherwise verb form. E.g

  • foo.normalization() return a number
  • foo.normalize() modify foo if necessary but don't return anything.

The former makes sense to chain together in English, as in foo.normalization().numerator(), the latter doesn't. Similar list.sort() vs. sorted(list) etc.

comment:9 follow-up: Changed 7 years ago by vbraun

And this method doesn't return anything, it just prints to the screen.

comment:10 in reply to: ↑ 9 Changed 7 years ago by nthiery

Replying to vbraun:

And this method doesn't return anything, it just prints to the screen.

Oh it does? Yikes! And I wrote this? Oops!

Ok, that explains the confusion; we agree on the naming conventions :-)

I'd rather have it return a string and use the idiom:

print ....plot().description()

so that we could reuse the result elsewhere in the of code.

Thanks!

comment:11 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:12 Changed 7 years ago by tscrim

  • Status changed from positive_review to needs_work

I've uploaded a review patch which changes describe() which prints to description() which returns a string as per Nicolas' request. Needs a review.

comment:13 Changed 7 years ago by tscrim

  • Status changed from needs_work to needs_review

comment:14 Changed 7 years ago by vbraun

  • Status changed from needs_review to positive_review

Thanks, looks good to me!

comment:15 follow-up: Changed 7 years ago by ppurka

Shouldn't the plot_expose function have been deprecated along with this change?

comment:16 Changed 7 years ago by vbraun

Was never imported into the global namespace, so it does not need deprecation.

comment:17 Changed 7 years ago by ppurka

Oh. I missed that. Seems combinat specific, but it is good to see it in the plot code.

comment:18 Changed 7 years ago by ppurka

Actually, this is quite a useful feature. Perhaps it should be documented somewhere in the plot code. Probably in the Graphics class of sage/plot/graphics.py. Need not be done in this ticket.

comment:19 in reply to: ↑ 15 Changed 7 years ago by jdemeyer

Replying to ppurka:

Shouldn't the plot_expose function have been deprecated along with this change?

Additionally, it was never in a released version of Sage (only betas), so there is no need for this.

comment:20 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.11.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.