Opened 7 years ago
Closed 6 years ago
#13067 closed enhancement (fixed)
is_strongly_regular
Reported by: | ncohen | Owned by: | jason, ncohen, rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-5.3 |
Component: | graph theory | Keywords: | strongly regular graph |
Cc: | wdj, kini, dimpase | Merged in: | sage-5.3.beta1 |
Authors: | Nathann Cohen | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12942, #12945, #12952, #12971, #12980, #12981, #12982, #12989, #13038, #13058 | Stopgaps: |
Description (last modified by )
Attachments (1)
Change History (27)
comment:1 follow-up: ↓ 3 Changed 7 years ago by
comment:2 Changed 7 years ago by
and one should also do "is_distance_regular()"!!! (strongly_regular=distance_regular, of diameter 2)
comment:3 in reply to: ↑ 1 Changed 7 years ago by
"certificate=" is misleading. This triple of numbers is called "parameters". I propose the optional argument be renamed so.
Hmmmm... Actually I wrote certificate = True
because maaaaaaaaany Graph functions now have a "certificate" parameter that gives more information when the function is expected to return True/False? answers. For instance is_bipartite(certificate = True) would return bipartitions or odd cycles, is_directed_acyclic would return a linear extension or a circuit, things like that. I agree that "certificate" is a bit weird in this context, but this is the reason why I named it like that.
Now that I gave you this reason tell me if you still want me to rename it to "parameters". If I did not convince you then the users will not find it convincing either and the best is to change it, and otherwise I will let it stay like that :-)
Nathann
comment:4 follow-up: ↓ 6 Changed 7 years ago by
"Certificate" has a quite specific meaning. A certificate of a certain property of an object is something which proves that the object has that property. A list of odd cycles would prove that a graph is not bipartite, and an explicit bipartition would prove that it is bipartite. A linear extension would prove that a graph is a DAG, and a circuit would prove that it is not a DAG. The triple you return is not in itself a proof of anything.
comment:5 Changed 7 years ago by
Also, nobody would expect a keyword argument called parameters
to be a boolean argument. Instead you should call it something like return_parameters
, IMO.
comment:6 in reply to: ↑ 4 Changed 7 years ago by
Replying to kini:
The triple you return is not in itself a proof of anything.
Yep. That's why I agree that it is improper, however it may still be better to use the same keyword as usual. I'll conform to your advice, so tell me what you think is best.
There is also this is_isomorphic(certify = True) and this automorphism_group(translation = True) that I do not like much :-)
Nathann
comment:7 Changed 7 years ago by
Please, stop putting spaces between keyword arguments and their values... I have made like a dozen reviewer patches removing those already :P
I think you should use return_parameters
for this . I see that .is_isomorphic(certify=True)
doesn't actually return a certificate when the graphs aren't isomorphic. Perhaps that one should be renamed in the future. But what's wrong with .automorphism_group(translation=True)
?
comment:8 Changed 7 years ago by
Well, return_parameters = True returns nothing when the graph is not strongly regular :-P
Nathann
comment:9 Changed 7 years ago by
- Status changed from new to needs_review
Here it is ! certificate
has become
return_parameters
:-)
About the spaces in the title. Quite honestly, rules about spaces and the length of lines are part of those things I classify as "the world's usual crazyness". Those rules, to me, make very few sense if any, and even if there is some reason behind it is not sufficient to create rules and force people to follow them. The world is crazy and totally paranoid.
In particular, I do not mind much if there are spaces at all or not, and if some lines have 85 characters instead of anything else people may agree on. If you want to rewrite my code according to the rules you think should be followed, I do not mind (note that you *would* disagree if I insisted on formatting the code in a different way). But all in all, I am fine with letting people live by the rules they fix for themselves, for as long as they do not force me to do the same.
Hence -- and I am sorry to say so -- I will forget again about those spaces, and I will not mind because there is to me no reason why I should really pay attention to that.
Oh. Actually, there is one. For instance you *do* mind, and it is for this very reason that I removed those spaces in the new patch. And I honestly do so uniquely to please you, so that you will not have to write another patch again to change it :-)
Sorry about all this, and I hope you understand my attitude toward these questions.
And thank you again for reviewing all these patches, and thank you again for taking the time to wonder whether certificate is the good word or should be replaced by something else.
See youuuuuuuuuuuuuuuuuu ! :-)
Nathann
comment:10 follow-up: ↓ 11 Changed 7 years ago by
It is all very well to paint yourself as a free spirit and me as a curmudgeonly "paranoid" grammar nazi who contributes to "the world's usual crazyness", but the fact remains that we have Sage coding conventions. Please follow them. If you don't want to, then complain to sage-devel, not me. Either way, I guess I will continue to write reviewer patches...
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to kini:
but the fact remains that we have Sage coding conventions
I do not, and no one ever asked me whether I did, hence I do not feel compelled to follow them. And of course I would leave any organization that forces me to follow crazy rules and is paranoid enough to believe it is better to refuse additions to the software because the lines do not have the proper length.
I will try to pay attention to my patches when you review them, because you review these patches out of kindness and I do not want it to create unnecessary work for you. But I have a very hard time remembering unreasonable rules, so please understand that I will do my best to write patches that suit your taste... But please, the same way that I do not take offense when you rewrite my patches in the way you think is right, do not be offended when I do not follow the rules you want to submit to.
Nathann
comment:12 Changed 7 years ago by
- Keywords strongly regular graph added
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
The code looks ok. I give a positive review, even if the bot only gives a red light.
comment:13 Changed 7 years ago by
comment:14 Changed 7 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:15 Changed 7 years ago by
- Status changed from positive_review to needs_work
This should be rebased to sage-5.2.beta1.
comment:16 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:17 Changed 7 years ago by
I have tried to rebase to 5.2.beta1. I hope I did this in a correct way..
comment:18 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:19 Changed 7 years ago by
- Milestone changed from sage-5.2 to sage-5.3
comment:21 Changed 6 years ago by
- Status changed from positive_review to needs_work
This should be rebased to sage-5.2.rc1 (sorry again).
comment:22 Changed 6 years ago by
- Status changed from needs_work to positive_review
Patch rebased on 5.2rc1. Will this patch go in 5.2 ?
comment:24 Changed 6 years ago by
The new patch needs a proper commit message.
Changed 6 years ago by
comment:25 Changed 6 years ago by
new patch with proper commit message
comment:26 Changed 6 years ago by
- Merged in set to sage-5.3.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
"certificate=" is misleading. This triple of numbers is called "parameters". I propose the optional argument be renamed so.