Opened 9 years ago

Closed 8 years ago

#10953 closed defect (fixed)

is_regular yields StopIterator error on empty graph

Reported by: pgdx Owned by: jason, ncohen, rlm
Priority: minor Milestone: sage-5.0
Component: graph theory Keywords: regular is_regular empty graph iterator
Cc: mjo Merged in: sage-5.0.beta1
Authors: Lukáš Lánský, Jeroen Demeyer Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The following statement yields an error:

sage: Graph().is_regular()

A quick fix to this is to test if the size of the graph is zero, in which case we return true. I assume the empty graph is k-regular for any k. If this is not the case for sage, another (better) error should be thrown.

Apply trac_10953_empty_graph_is_regular.4.patch

Attachments (4)

trac_10953_empty_graph_is_regular.patch (833 bytes) - added by brunellus 8 years ago.
trac_10953_empty_graph_is_regular.2.patch (857 bytes) - added by brunellus 8 years ago.
trac_10953_empty_graph_is_regular.3.patch (916 bytes) - added by brunellus 8 years ago.
trac_10953_empty_graph_is_regular.4.patch (760 bytes) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by brunellus

comment:1 Changed 8 years ago by brunellus

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by mjo

  • Cc mjo added

You should include the ticket number in the doctest.

I would test self.order() rather than len(self) to save the reader from having to go look up what __len__ does, but that's just personal taste.

Changed 8 years ago by brunellus

comment:3 Changed 8 years ago by brunellus

Thanks! Is it better now?

comment:4 Changed 8 years ago by mjo

Almost! Would you mind moving the comment to its own line? There's nothing wrong with it at the moment, but in-line comments like that are given special treatment (e.g. #long, #optional, #todo...) so I think they should be avoided to prevent unexpected interactions.

Also: I find regularity defined "for all vertices..." so it is vacuously true that the empty graph is k-regular for any k.

I'm running a full ptestlong with the patch, and don't expect any problems.

comment:5 Changed 8 years ago by brunellus

I agree that the empty graph is k-regular for any k -- I think the change works that way now. Or do you have in mind that I write another test for this?

comment:6 Changed 8 years ago by mjo

Nope, it's fine. I wasn't sure how regularity was defined, and your description said "I assume the empty graph is k-regular for any k", so I looked it up.

Changed 8 years ago by brunellus

comment:7 Changed 8 years ago by brunellus

Is this way of commenting possible?

Thanks for all the help. I'll try to not repeat those errors in my next patches. :-)

comment:8 Changed 8 years ago by mjo

  • Description modified (diff)
  • Reviewers set to Michael Orlitzky
  • Status changed from needs_review to positive_review

Perfect.

comment:9 Changed 8 years ago by brunellus

  • Authors set to Lukáš Lánský

comment:10 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:11 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Milestone set to sage-5.0
  • Status changed from needs_work to needs_review

Changed 8 years ago by jdemeyer

comment:12 Changed 8 years ago by jdemeyer

  • Authors changed from Lukáš Lánský to Lukáš Lánský, Jeroen Demeyer

Small fix to the doctest, needs review.

comment:13 Changed 8 years ago by mjo

  • Status changed from needs_review to positive_review

It passed another ptestlong, and the example looks good in the docs.

Sorry I missed the backticks, I finally fixed my dvipng and created a review checklist so I won't miss them in the future.

comment:14 Changed 8 years ago by jdemeyer

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