Opened 7 years ago

Closed 7 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 chapoton)

I can't believe we still did not have this one ! ;-)

Nathann

apply only trac_13067-new.patch

Attachments (1)

trac_13067-new.patch (4.0 KB) - added by chapoton 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 follow-up: Changed 7 years ago by dimpase

"certificate=" is misleading. This triple of numbers is called "parameters". I propose the optional argument be renamed so.

comment:2 Changed 7 years ago by dimpase

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 ncohen

"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: Changed 7 years ago by kini

"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 kini

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 ncohen

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 kini

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 ncohen

Well, return_parameters = True returns nothing when the graph is not strongly regular :-P

Nathann

comment:9 Changed 7 years ago by ncohen

  • 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: Changed 7 years ago by kini

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 ncohen

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 chapoton

  • Authors set to Nathan Cohen
  • 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 chapoton

  • Authors changed from Nathan Cohen to Nathann Cohen

comment:14 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:15 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This should be rebased to sage-5.2.beta1.

comment:16 Changed 7 years ago by chapoton

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

comment:17 Changed 7 years ago by chapoton

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 dimpase

  • Status changed from needs_review to positive_review

comment:19 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-5.3

comment:20 Changed 7 years ago by chapoton

  • Description modified (diff)

apply only trac_13067-new.patch

comment:21 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This should be rebased to sage-5.2.rc1 (sorry again).

comment:22 Changed 7 years ago by chapoton

  • Status changed from needs_work to positive_review

Patch rebased on 5.2rc1. Will this patch go in 5.2 ?

comment:23 Changed 7 years ago by chapoton

  • Description modified (diff)

comment:24 Changed 7 years ago by jdemeyer

The new patch needs a proper commit message.

Changed 7 years ago by chapoton

comment:25 Changed 7 years ago by chapoton

new patch with proper commit message

comment:26 Changed 7 years ago by jdemeyer

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