Opened 7 years ago
Closed 7 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 )
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)
Change History (13)
comment:1 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
comment:6 Changed 7 years ago by
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:8 Changed 7 years ago by
Ahaahaaha ! No problem :-)
Nathann
comment:9 Changed 7 years ago by
- Status changed from needs_review to positive_review
Everything is ok, no need for deprecation..
Positive review
comment:10 Changed 7 years ago by
Wouhouuuuuuuuuuu !! THaaaaaaaaaaaaaaankss !! :-)
Nathann
comment:11 Changed 7 years ago by
- Milestone changed from sage-5.6 to sage-5.7
- Reviewers set to Nathann Cohen
comment:12 Changed 7 years ago by
- Merged in set to sage-5.7.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
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.
coloring.py
that look totally unrelated. Should they be somewhere else?because all they want is
lambda
. Now they will getk
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.) Justparameters
would be cleaner, imho.Rob