Opened 6 years ago

Closed 6 years ago

#14442 closed defect (fixed)

Graph.is_circular_planar does not behave as expected

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.10
Component: graph theory Keywords:
Cc: Merged in: sage-5.10.rc0
Authors: Nathann Cohen Reviewers: Nico Van Cleemput
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

As reported there https://groups.google.com/d/topic/sage-edu/ivWea0Gk3_Q/discussion

By the way, nobody would expect this {{{ sage: graphs.CompleteBipartiteGraph?(3,2).is_circular_planar() True }}}

The same way that nobody would expect Sage to return "True" when is_planar is called on K_5. And this was one of the doctests -_-

Nathann

Attachments (1)

trac_14442.patch (12.3 KB) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by nvcleemp

  • Reviewers set to Nico Van Cleemput
  • Status changed from needs_review to needs_info

Seems OK. Everything works as I expect it to work, the tests pass and the documentation is clearer. The only thing I was wondering about is the reference which says: '[Online] Available: soon!' You might not now whether this is available in the meantime and where? I couldn't find anything.

comment:3 Changed 6 years ago by ncohen

I searched around and it does not seem to exist anywhere :-/

Nathann

comment:4 Changed 6 years ago by ncohen

  • Status changed from needs_info to needs_review

comment:5 Changed 6 years ago by nvcleemp

  • Status changed from needs_review to positive_review

OK, that was also my conclusion. :-(

But that doesn't affect the validity of this patch.

comment:6 follow-up: Changed 6 years ago by jdemeyer

dochtml.log:[graphs   ] /mazur/release/merger/sage-5.10.rc0/local/lib/python2.7/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph.GenericGraph.is_circular_planar:47: ERROR: Unknown target name: "2".

comment:7 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Changed 6 years ago by ncohen

comment:8 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

Could you please check that this patch is correct ? I updated this cursed reference [2] to respect Sphinx' format.

Nathann

comment:9 in reply to: ↑ 6 Changed 6 years ago by nvcleemp

  • Status changed from needs_review to positive_review

Replying to jdemeyer:

dochtml.log:[graphs   ] /mazur/release/merger/sage-5.10.rc0/local/lib/python2.7/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph.GenericGraph.is_circular_planar:47: ERROR: Unknown target name: "2".

Aaargh, I had expected ./sage -docbuild reference html to summarise any errors encountered at the end. But I guess this is not the case. Luckily, I meanwhile discovered ./sage -docbuild reference/graphs html. The rest is still the same, so OK for me.

comment:10 Changed 6 years ago by ncohen

Thanks ! :-)

Nathann

comment:11 Changed 6 years ago by jdemeyer

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