Opened 5 years ago
Closed 5 years ago
#17108 closed enhancement (fixed)
IncidenceStructure.degree(set={1,2,3})
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  combinatorial designs  Keywords:  
Cc:  vdelecroix, dimpase  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Vincent Delecroix, Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  527bbc1 (Commits)  Commit:  527bbc11155bc54479f53532d82ed7f774dfc829 
Dependencies:  #17104  Stopgaps: 
Description
Degree of a set, degree of all ksubsets of points.
Nathann
Change History (41)
comment:1 Changed 5 years ago by
 Branch set to u/ncohen/17108
 Dependencies set to #17104
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Commit set to c34fda620e898708392becbbf8994d9065e29371
comment:3 followup: ↓ 4 Changed 5 years ago by
Hey,
+ if int(p is None)+int(set is None)+int(sets_of_size is None) <2: + raise ValueError("Only one parameter must be defined"
wouldn't >1
be more appropriate? (I note that you used a ValueError
and not an assert ;)
Hum
+ if set is not None: + # getting the 'set' function back + set_of_points = set + from __builtin__ import set
looks like another argument name would be better... points
? pts
?
Vincent
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 5 years ago by
Yo !
wouldn't
>1
be more appropriate?
O_o
Well it wouldn't work in that case.
(I note that you used a
ValueError
and not an assert ;)
:P
looks like another argument name would be better...
points
?pts
?
point
is the current one. I want to make clear that the meaning of the function changes, so I thought that "set" would do. It cannot be detected automatically btw.
Nathann
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 5 years ago by
Replying to ncohen:
looks like another argument name would be better...
points
?pts
?
point
is the current one. I want to make clear that the meaning of the function changes, so I thought that "set" would do. It cannot be detected automatically btw.
I meant instead of set
; use something else like points
or pts
. Note that I assumed that the following is ugly
def f(list, int): return __builtin__.list(list) * __builtin__.int(int)
if you are not convinced, try first __builtin__ = 3
in the console.
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 5 years ago by
I meant instead of
set
; use something else likepoints
orpts
.
That's what I understood. Right now we have 'point', it would be dangerous to have 'points' and weird to have 'pts'.
Note that I assumed that the following is ugly
What does this code do ?
Nathann
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 5 years ago by
Replying to ncohen:
I meant instead of
set
; use something else likepoints
orpts
.That's what I understood. Right now we have 'point', it would be dangerous to have 'points' and weird to have 'pts'.
Right now
 def degree(self, p=None): + def degree(self, p=None, set=None, sets_of_size=None):
There is no point
...
Note that I assumed that the following is ugly
What does this code do ?
So you agree that it is unreadable ;)
Vincent
comment:8 in reply to: ↑ 7 Changed 5 years ago by
Yo !
Right now
 def degree(self, p=None): + def degree(self, p=None, set=None, sets_of_size=None):There is no
point
...
Oh sorry. But p=None,pts=None
or p=None,points=None
is not more meaningful either.
So you agree that it is unreadable ;)
Yes. What's the point ?
I just want the user interface to make sense. The meaning of I.degree(p)
is clear, the meaning of I.degree(set=[1,2,3])
is not optimal but contains as much information as I could put, and I.degree(sets_of_size=3)
is as clear as possible too.
I thought of I.degree(subset=[1,2,3])
but did not like it much. I don't see what the 'sub' does there.
So well. I am sorry if it overwrites a 'set' keyword in the function (and if, unluckily, we need it) but well... Why is that even a problem ? I rename is as early as possible to make the code clearer.
Nathann
comment:9 Changed 5 years ago by
 Cc dimpase added
comment:10 Changed 5 years ago by
 Commit changed from c34fda620e898708392becbbf8994d9065e29371 to 549aefadbe387c1c4d4c418304e5cc5430d34ebb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
549aefa  trac #17108: IncidenceStructure.degree(set={1,2,3})

comment:11 Changed 5 years ago by
Rebased on rc0.
Okay, so you told me that you did not like the "set=" argument, but how can I make it better ? I want the interface to be clear and natural for users.
Nathann
comment:12 Changed 5 years ago by
 Commit changed from 549aefadbe387c1c4d4c418304e5cc5430d34ebb to f82b9c41095b538a5da29ba015264c24adddc7c4
Branch pushed to git repo; I updated commit sha1. New commits:
f82b9c4  trac #17108: Possibly better

comment:13 followup: ↓ 14 Changed 5 years ago by
The thing that I dislike is that you use the argument name set
. Moreover you need to use Python sets inside the function.
You used p
for point
why not use s
for set
?
Vincent
comment:14 in reply to: ↑ 13 Changed 5 years ago by
The thing that I dislike is that you use the argument name
set
. Moreover you need to use Python sets inside the function.
I tried to make it a little better in the last commit
You used
p
forpoint
why not uses
forset
?
Because I.degree(s={1,2,3})
is not as clear as I.degree(set={1,2,3})
is. And the p
for point
is not really a problem because nobody ever types it, i.e. I.degree(0)
.
Nathann
comment:15 Changed 5 years ago by
Helloooo !! Could we do something about that ticket ? I really need this feature pretty often and I write a long oneliner each time I do :P
Nathann
comment:16 Changed 5 years ago by
Beep ?...
Nathann
comment:17 followup: ↓ 18 Changed 5 years ago by
I don't like the API:
degree(self, p=None, set=None, sets_of_size=None)
looks very nonintuitive (Why p
? Why set
?!). I'd suggest
degree(self, *args, sets_of_size=None)
and args are either your p
or your set
, or None.
comment:18 in reply to: ↑ 17 ; followup: ↓ 19 Changed 5 years ago by
Hello !
looks very nonintuitive (Why
p
? Whyset
?!). I'd suggest
You are meant to use H.degree(a_point), H.degree(set=set_of_points), H.degree(sets_of_size=3)
degree(self, *args, sets_of_size=None)and args are either your
p
or yourset
, or None.
I cannot do that, for I may have both 1
and {1}
in the incidence structure.
Nathann
comment:19 in reply to: ↑ 18 ; followup: ↓ 20 Changed 5 years ago by
Replying to ncohen:
Hello !
looks very nonintuitive (Why
p
? Whyset
?!). I'd suggestYou are meant to use
H.degree(a_point), H.degree(set=set_of_points), H.degree(sets_of_size=3)
degree(self, *args, sets_of_size=None)and args are either your
p
or yourset
, or None.I cannot do that, for I may have both
1
and{1}
in the incidence structure.
blah.degree(1)==blah.degree({1})
need not hold, as
isinstance({1},set)==True
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 5 years ago by
What I am saying is that in this case, I do not know what do do:
sage: H = IncidenceStructure([[1,2],[1,Set([1])]]) sage: H.ground_set() [{1}, 1, 2] sage: H.blocks() [[{1}, 1], [1, 2]] sage: H.degree(Set([1]))
Nathann
comment:21 in reply to: ↑ 20 ; followup: ↓ 22 Changed 5 years ago by
Replying to ncohen:
What I am saying is that in this case, I do not know what do do:
sage: H = IncidenceStructure([[1,2],[1,Set([1])]]) sage: H.ground_set() [{1}, 1, 2] sage: H.blocks() [[{1}, 1], [1, 2]] sage: H.degree(Set([1]))
well, at least in set theory 1 is not equal to {1}. Thusly the answer should be 1, as opposed to H.degree(1)
, which should be 2.
comment:22 in reply to: ↑ 21 ; followup: ↓ 23 Changed 5 years ago by
well, at least in set theory 1 is not equal to {1}. Thusly the answer should be 1, as opposed to
H.degree(1)
, which should be 2.
What I am saying is that from the command there is no way if by H.degree(Set([1]))
the user means "give me the degree of the point Set([1])" or "give me the degree of the set that only contain the point 1".
This is why I need this "set=" parameter to make the difference.
Nathann
comment:23 in reply to: ↑ 22 ; followup: ↓ 24 Changed 5 years ago by
Replying to ncohen:
well, at least in set theory 1 is not equal to {1}. Thusly the answer should be 1, as opposed to
H.degree(1)
, which should be 2.What I am saying is that from the command there is no way if by
H.degree(Set([1]))
the user means "give me the degree of the point Set([1])" or "give me the degree of the set that only contain the point 1".This is why I need this "set=" parameter to make the difference.
Ah, right. Then I'd just go for degree()
(for points) and subset_degree()
(for subsets), to avoid dealing with paradoxes of set theory (;).
comment:24 in reply to: ↑ 23 ; followup: ↓ 25 Changed 5 years ago by
Ah, right. Then I'd just go for
degree()
(for points) andsubset_degree()
(for subsets), to avoid dealing with paradoxes of set theory (;).
Two functions ?... :/
I would prefer degree(subset=the_set)
if you do not mind. Even though I find degree(set=the_set)
much more natural, but nobody likes it.
Would that be okay for you ?
Nathann
comment:25 in reply to: ↑ 24 Changed 5 years ago by
Replying to ncohen:
Ah, right. Then I'd just go for
degree()
(for points) andsubset_degree()
(for subsets), to avoid dealing with paradoxes of set theory (;).Two functions ?...
:/
It's more efficient, especially if you compile (in some future version), isn't it? And, otherwise it feels like COBOL :)
I would prefer
degree(subset=the_set)
if you do not mind. Even though I finddegree(set=the_set)
much more natural, but nobody likes it.Would that be okay for you ?
and what will happen to p=
? It should go...
comment:26 followup: ↓ 27 Changed 5 years ago by
+  ``sets_of_size``  an integer. If defined, the function returns the + degree of all sets of size ``sets_of_size``.
should be degrees
there...
Anyhow, I think this should also be a separate function, called degrees
.
Then degrees(1)
or degrees()
would to what your degree()
currently does.
You just can't stuff parameters with such different semantics into one function, unless you want your users to get a headache...
comment:27 in reply to: ↑ 26 ; followup: ↓ 28 Changed 5 years ago by
should be
degrees
there...
Dima please, let's not create 9999 functions which all do the same thing. Look, we have had this in graphs for ages and nobody died:
sage: g = graphs.PetersenGraph() sage: g.degree(1) 3 sage: g.degree() [3, 3, 3, 3, 3, 3, 3, 3, 3, 3] sage: g.degree(labels=True) {0: 3, 1: 3, 2: 3, 3: 3, 4: 3, 5: 3, 6: 3, 7: 3, 8: 3, 9: 3}
Anyhow, I think this should also be a separate function, called
degrees
. Thendegrees(1)
ordegrees()
would to what yourdegree()
currently does.
1year deprecation ?
Dima tell me: do you have an objection if I change degree(set=the_set)
to degree(subset=the_set)
? And I will remove sets_of_size
if you refuse it, it is better than a "degrees" in my mind.
Nathann
comment:28 in reply to: ↑ 27 ; followup: ↓ 29 Changed 5 years ago by
Replying to ncohen:
should be
degrees
there...Dima please, let's not create 9999 functions which all do the same thing.
there are already 9998 functions like this there in generic_graph.py
:
degree
, degree_sequence
, degree_histogram
, etc...
You might compare implementations of the 1st two and try to convince me that this is not 100% code/API bloat... IMHO something should be done about this.
comment:29 in reply to: ↑ 28 ; followup: ↓ 30 Changed 5 years ago by
there are already 9998 functions like this there in
generic_graph.py
:degree
,degree_sequence
,degree_histogram
, etc...
I would remove the last two in I could, but it is not my personnal code.
Can you answer the question about "set=" or "subset=" ?
Nathann
comment:30 in reply to: ↑ 29 Changed 5 years ago by
Replying to ncohen:
there are already 9998 functions like this there in
generic_graph.py
:degree
,degree_sequence
,degree_histogram
, etc...I would remove the last two in I could, but it is not my personnal code.
I'd rather remove the functionality of degree_sequence
from degree
.
It's apparently written by rlm, I doubt he'd object.
Can you answer the question about "set=" or "subset=" ?
surely subset
is better (and please, please do not name variables/parameters set
, for Pete's sake...)
And yes, please get rid of p=
and of sets_of_size=
.
comment:31 Changed 5 years ago by
but, really, it is totally wrong to have 3 mutually exclusive keyword parameters A, B, C f(A=bla)
, f(B=fook)
, f(C=fok)
and, surely enough, code in f
to parse all this crap, so that people do not mistakenly use them together,
instead of fA(bla)
, fB(fook)
, fC(fok)
.
This is not what the keyword parameters are for, they are not poor man substitutes of function names. No, they are not...
No, really. Take a deep breath an think about it for 3 minutes...
comment:32 followup: ↓ 34 Changed 5 years ago by
surely subset is better (and please, please do not name variables/parameters set, for Pete's sake...)
It is only a problem in your head, not a problem in the code. With two lines everything is solved, and the user interface is clearer as a result.
+ S = set + from __builtin__ import set
That's all it takes.
And yes, please get rid of
p=
and ofsets_of_size=
.
Too bad, I liked the sets_of_size
feature.
but, really, it is totally wrong to have 3 mutually exclusive keyword parameters
We did almost the same in PermutationGroup.orbit
. Instead of different flags we had a action='OnPoints',action='OnSets', ...
. I would not mind having IncidenceStructure.degree(p,type='set')
or type='point'
but you would not like it.
This is not what the keyword parameters are for, they are not poor man substitutes of function names. No, they are not...
You seem really bothered by what I do with those parameters. Are you worried that users may find it confusing ?
Nathann
comment:33 followup: ↓ 35 Changed 5 years ago by
Hello,
I agree with Dima that set
and type
should be avoided as argument or variable name.
Concerning the number of functions needed, the only ambiguity I see is that .degree()
can return a list of degrees (so that it would be more appropriate to call it .degrees()
). What about
degree(self, data, subset=None)
: return the degree of a point or a subset (ifNone
we assume that there is no amiguity, otherwise it must be set toTrue
orFalse
).degrees(self, size=1)
: return the set of degrees of points. Ifsize
is provided with an integer > 1 then it returns the degrees of subsets of that given size.
Vincent
comment:34 in reply to: ↑ 32 Changed 5 years ago by
Replying to ncohen:
surely subset is better (and please, please do not name variables/parameters set, for Pete's sake...)
It is only a problem in your head, not a problem in the code. With two lines everything is solved, and the user interface is clearer as a result.
+ S = set + from __builtin__ import setThat's all it takes.
until someone decides to change the code, and adds stuff before S=
...
And yes, please get rid of
p=
and ofsets_of_size=
.Too bad, I liked the
sets_of_size
feature.but, really, it is totally wrong to have 3 mutually exclusive keyword parameters
We did almost the same in
PermutationGroup.orbit
. Instead of different flags we had aaction='OnPoints',action='OnSets', ...
. I would not mind havingIncidenceStructure.degree(p,type='set')
ortype='point'
but you would not like it.
The only reason it is done so in PermutationGroup.orbit
is that action
can be userdefined (at least in the underlying GAP code).
(for the record, there are currently 8 different actions available, and needless
to say, they all dispatch to the same GAP call in the code).
And here, unlike there, runtime checking and dispatching on the type of parameters is 100% code bloat.
This is not what the keyword parameters are for, they are not poor man substitutes of function names. No, they are not...
You seem really bothered by what I do with those parameters. Are you worried that users may find it confusing ?
As a reviewer, I find it totally wrong design. Many users would be confused, too.
comment:35 in reply to: ↑ 33 Changed 5 years ago by
Vincent:
Does the new commit suit your taste ?
As a reviewer, I find it totally wrong design. Many users would be confused, too.
I really believe that H.degree(set={1,2,3})
is rather selfexplaining.
Nathann
comment:36 Changed 5 years ago by
 Commit changed from f82b9c41095b538a5da29ba015264c24adddc7c4 to f4f8dd6ee1c6ad08385cd1e0d00d2457cade86a4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f4f8dd6  trac #17108: IncidenceStructure.degree(subset=1) and .degrees()

comment:37 followup: ↓ 38 Changed 5 years ago by
 Branch changed from u/ncohen/17108 to u/vdelecroix/17108
 Commit changed from f4f8dd6ee1c6ad08385cd1e0d00d2457cade86a4 to 527bbc11155bc54479f53532d82ed7f774dfc829
Hi,
Thanks for the modification. Looks like almost perfect ;)
Beyond docs, I added the following change in my commit
Python is cool:
sage: sum([True]) 1 sage: sum([True,False,False,True,False]) 2
So instead of sum(1 for x in X if condition(x))
one can write sum(condition(x) for x in X)
.
I do not think we care here, but it has incidence
sage: l = [choice([True,False]) for _ in range(10000)] sage: %timeit sum(l) 1000 loops, best of 3: 147 µs per loop sage: %timeit sum(i for i in l) 1000 loops, best of 3: 497 µs per loop sage: %timeit sum(1 for i in l if i) 1000 loops, best of 3: 826 µs per loop
I let Dima make more comments but on my side it would be ok to give positive review.
Vincent
New commits:
527bbc1  trac #17108: doc and sums

comment:38 in reply to: ↑ 37 ; followup: ↓ 39 Changed 5 years ago by
Replying to vdelecroix:
Hi,
Thanks for the modification. Looks like almost perfect ;)
indeed. I'll have another look later today, but it looks like it should be OK.
comment:39 in reply to: ↑ 38 Changed 5 years ago by
indeed. I'll have another look later today, but it looks like it should be OK.
Fine for me ! Dima, you can change the status whenever you are convinced.
Nathann
comment:40 Changed 5 years ago by
 Reviewers set to Vincent Delecroix, Dima Pasechnik
 Status changed from needs_review to positive_review
comment:41 Changed 5 years ago by
 Branch changed from u/vdelecroix/17108 to 527bbc11155bc54479f53532d82ed7f774dfc829
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17104: IncidenceStructure.relabel() (no arguments)
trac #17108: IncidenceStructure.degree(set={1,2,3})