Opened 6 years ago
Closed 6 years ago
#18646 closed enhancement (fixed)
Explicitly say that arguments to Graph.plot() are forwarded
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  graph theory  Keywords:  
Cc:  slabbe, jmantysalo  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Jori Mäntysalo 
Report Upstream:  N/A  Work issues:  
Branch:  f3a61f7 (Commits)  Commit:  f3a61f76ab8f28c6e50bd66cfaf392c61b421d6d 
Dependencies:  Stopgaps: 
Description
As reported in [1], Graph.plot() is not sufficiently explicit about the arguments it accepts.
Nathann
[1] https://groups.google.com/d/topic/sagedevel/XS0EDLqfGtI/discussion
Change History (10)
comment:1 Changed 6 years ago by
 Branch set to public/18646
 Commit set to 228ff6bd7077c14c365df19ad5fd5b24f10b93d9
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Reviewers set to Jori Mäntysalo
 Status changed from needs_review to positive_review
LGTM.
comment:3 Changed 6 years ago by
Thanks !
comment:4 followup: ↓ 5 Changed 6 years ago by
I wanted to review this ticket this morning, but it is already positive review. I had one english comment. I don't understand the part "to which they are forwarded.
" in
 This method supports any parameter accepted by :meth:`sage.plot.graphics.Graphics.show`, to which they are forwarded.
Because arguments are forwarded *from* show *to* plot not the converse. I would replace "to which" by "from which" or better erase completely "to which they are forwarded.
" because the user don't care much about what is forwarded where.
I let you guys decide if it is too late or not (maybe this ticket is already being merged).
comment:5 in reply to: ↑ 4 ; followup: ↓ 8 Changed 6 years ago by
Replying to slabbe:
I wanted to review this ticket this morning, but it is already positive review. I had one english comment. I don't understand the part "
to which they are forwarded.
" in This method supports any parameter accepted by :meth:`sage.plot.graphics.Graphics.show`, to which they are forwarded.Because arguments are forwarded *from* show *to* plot not the converse.
Nonono. The function is Graphics.show
, not Graph.show
. And in particular Graph.plot
forwards its arguments to Graphics.show
. This is why, despite having no specific code in Graph.plot
to handle "axis", that argument is accepted. Because Graphics.show
knows what to make of it.
I let you guys decide if it is too late or not (maybe this ticket is already being merged).
I can remove the sentence if you prefer. In the worst case, the last commit will be ignored by Volker's script.
Nathann
comment:6 Changed 6 years ago by
 Commit changed from 228ff6bd7077c14c365df19ad5fd5b24f10b93d9 to f3a61f76ab8f28c6e50bd66cfaf392c61b421d6d
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
f3a61f7  trac #18646: Behead a sentence.

comment:7 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:8 in reply to: ↑ 5 ; followup: ↓ 9 Changed 6 years ago by
Replying to ncohen:
Nonono. The function is
Graphics.show
, notGraph.show
. And in particularGraph.plot
forwards its arguments toGraphics.show
. This is why, despite having no specific code inGraph.plot
to handle "axis", that argument is accepted. BecauseGraphics.show
knows what to make of it.
Ok, I see what you meant. But, I still think the last commit it a good idea to avoid confusion.
So if I understand correctly, that "forward" is done indirectly in the ugly line 910 in the method def plot(self, **kwds):
of file graph_plot.py:
G._set_extra_kwds(Graphics._extract_kwds_for_show(options))
comment:9 in reply to: ↑ 8 Changed 6 years ago by
So if I understand correctly, that "forward" is done indirectly in the ugly line 910 in the method
def plot(self, **kwds):
of file graph_plot.py:
Yesyes, it seems so.
Nathann
comment:10 Changed 6 years ago by
 Branch changed from public/18646 to f3a61f76ab8f28c6e50bd66cfaf392c61b421d6d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #18646: Explicitly say that arguments to Graph.plot() are forwarded