is_strongly_regular

[ncohen]

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

Nathann

apply only trac_13067-new.patch

[dimpase]

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

[dimpase]

and one should also do "is_distance_regular()"!!! (strongly_regular=distance_regular, of diameter 2)

[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

[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.

[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.

[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

[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)?

[ncohen]

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

Nathann

[ncohen]

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

[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...

[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

[chapoton]

The code looks ok. I give a positive review, even if the bot only gives a red light.

[jdemeyer]

This should be rebased to sage-5.2.beta1.

[chapoton]

I have tried to rebase to 5.2.beta1. I hope I did this in a correct way..

[chapoton]

apply only trac_13067-new.patch

[jdemeyer]

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

[chapoton]

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

[chapoton]

apply only trac_13067-new.patch

[jdemeyer]

The new patch needs a proper commit message.

[chapoton]

new patch with proper commit message