Opened 5 years ago

Closed 5 years ago

#13510 closed enhancement (fixed)

Small change to graph_is_strongly_regular (return the number of vertices)

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.7
Component: graph theory Keywords:
Cc: kcrisman, Merged in: sage-5.7.beta1
Authors: Nathann Cohen, Geoff Tims Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ncohen)

Geoff Tims mentionned aloud that it would make more sense for is_strongly_regular to return quadruples instead of triples : the current method does not return the number of vertices which -- although it is not hard to get in Sage -- is an important information on a strongly regular graph.

This small change is contained in the present patch. Along with the remark from http://ask.sagemath.org/question/1782/minor-improvement-to-chromatic-number

Nathann

APPLY:

Attachments (1)

trac_13510.patch (4.7 KB) - added by ncohen 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by ncohen

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by rbeezer

I'd say this is the right thing to return, it is extremely standard to quote all four at once. At a minimum it is a big improvement. Two initial comments.

  1. Patch has some changes to coloring.py that look totally unrelated. Should they be somewhere else?
  2. What if somebody currently does
    g.is_strongly_regular(return_parameters = True)[1]
    

because all they want is lambda. Now they will get k and be none the wiser. How about deprecating the current keyword and make up a new (similar) one? Or maybe there is a better fix that I am not thinking of right now. (There is an easy decorator for deprecating a keyword, iirc.) Just parameters would be cleaner, imho.

Rob

comment:3 Changed 5 years ago by ncohen

Yoooooooooooooo !!!

Well, the changes to coloring.py are so unimportant that they do not really deserve their own patch, it's almost a typo except it worked anyway :-P

For your other problem : the function is so new that I guess almost noones has noticed it yet. If you think that parameters = True would be better than return_parameters = True then it almost solves the problem because an old g.is_strongly_regular(return_parameters = True)[1] would not work anymore. So what would you think if I updated my patch with return_parameters -> parameters and let it stand like that ? The deprecation machinery looks like overkill in this case... I should have written the function like that from the start, forgot it, well... Nothing really important :-)

Nathann

comment:4 Changed 5 years ago by rbeezer

I did not realize it was new.

I do like parameters better and just totally breaking the old keyword quickly would be fine with me. But others might see it different. I think you can do deprecation in one line as a decorator, though, so where is the harm?

See sage.misc.decorators.rename_keyword(deprecated=None, deprecation=None, **renames) - its trivial.

Rob

comment:5 Changed 5 years ago by ncohen

Well, because the deprecation will be forgotten for a year or so, and it will require another patch to remove it :-P

I see it as "I create a patch to fix something, but I will have to write another patch in a long time, and I know that I will forget" :-P

Nathann

Changed 5 years ago by ncohen

comment:6 Changed 5 years ago by ncohen

Helloooooooooo !!

I just updated the patch so that return_parameters is now parameters, and added something in the documentation of spanning_trees.pyx. I tried to use the rename_keyword thing and I do not really see the point : it looks like renaming an argument with a deprecation just makes both names aliases to each other, so it is better to rename the code immediately. Besides, this way the documentation is consistent (otherwise users can still use return_parameters while it does not appear in the method's doc, that's weird O_o)

Nathann

comment:7 Changed 5 years ago by kcrisman

  • Authors changed from Nathann Cohen to Nathann Cohen, Geoff Tims

Just adding credit for the bit from ask.sagemath, if that's ok :)

comment:8 Changed 5 years ago by ncohen

Ahaahaaha ! No problem :-)

Nathann

comment:9 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

Everything is ok, no need for deprecation..

Positive review

comment:10 Changed 5 years ago by ncohen

Wouhouuuuuuuuuuu !! THaaaaaaaaaaaaaaankss !! :-)

Nathann

comment:11 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.6 to sage-5.7
  • Reviewers set to Nathann Cohen

comment:12 Changed 5 years ago by jdemeyer

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