Opened 4 years ago

Closed 4 years ago

#18646 closed enhancement (fixed)

Explicitly say that arguments to Graph.plot() are forwarded

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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/sage-devel/XS0EDLqfGtI/discussion

Change History (10)

comment:1 Changed 4 years ago by ncohen

  • Branch set to public/18646
  • Commit set to 228ff6bd7077c14c365df19ad5fd5b24f10b93d9
  • Status changed from new to needs_review

New commits:

228ff6btrac #18646: Explicitly say that arguments to Graph.plot() are forwarded

comment:2 Changed 4 years ago by jmantysalo

  • Reviewers set to Jori Mäntysalo
  • Status changed from needs_review to positive_review

LGTM.

comment:3 Changed 4 years ago by ncohen

Thanks !

comment:4 follow-up: Changed 4 years ago by 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. 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 ; follow-up: Changed 4 years ago by ncohen

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 4 years ago by git

  • 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:

f3a61f7trac #18646: Behead a sentence.

comment:7 Changed 4 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:8 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by slabbe

Replying to ncohen:

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.

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 4 years ago by ncohen

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 4 years ago by vbraun

  • Branch changed from public/18646 to f3a61f76ab8f28c6e50bd66cfaf392c61b421d6d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.