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: sage-6.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 k-subsets of points.

Nathann

Change History (41)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/17108
  • Dependencies set to #17104
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to c34fda620e898708392becbbf8994d9065e29371

Branch pushed to git repo; I updated commit sha1. New commits:

795cc45trac #17104: IncidenceStructure.relabel() (no arguments)
c34fda6trac #17108: IncidenceStructure.degree(set={1,2,3})

comment:3 follow-up: Changed 5 years ago by vdelecroix

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 ; follow-up: Changed 5 years ago by ncohen

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 ; follow-up: Changed 5 years ago by vdelecroix

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 ; follow-up: Changed 5 years ago by ncohen

I meant instead of set; use something else like points or pts.

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 ; follow-up: Changed 5 years ago by vdelecroix

Replying to ncohen:

I meant instead of set; use something else like points or pts.

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 ncohen

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 ncohen

  • Cc dimpase added

comment:10 Changed 5 years ago by git

  • Commit changed from c34fda620e898708392becbbf8994d9065e29371 to 549aefadbe387c1c4d4c418304e5cc5430d34ebb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

549aefatrac #17108: IncidenceStructure.degree(set={1,2,3})

comment:11 Changed 5 years ago by ncohen

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 git

  • Commit changed from 549aefadbe387c1c4d4c418304e5cc5430d34ebb to f82b9c41095b538a5da29ba015264c24adddc7c4

Branch pushed to git repo; I updated commit sha1. New commits:

f82b9c4trac #17108: Possibly better

comment:13 follow-up: Changed 5 years ago by vdelecroix

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 ncohen

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 for point why not use s for set?

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 ncohen

Helloooo !! Could we do something about that ticket ? I really need this feature pretty often and I write a long one-liner each time I do :-P

Nathann

comment:16 Changed 5 years ago by ncohen

Beep ?...

Nathann

comment:17 follow-up: Changed 5 years ago by dimpase

I don't like the API:

degree(self, p=None, set=None, sets_of_size=None)

looks very non-intuitive (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 ; follow-up: Changed 5 years ago by ncohen

Hello !

looks very non-intuitive (Why p? Why set?!). 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 your set, 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 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

Hello !

looks very non-intuitive (Why p? Why set?!). 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 your set, 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 ; follow-up: Changed 5 years ago by 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]))

Nathann

comment:21 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by dimpase

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 ; follow-up: Changed 5 years ago by 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.

Nathann

comment:23 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by dimpase

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 ; follow-up: Changed 5 years ago by ncohen

Ah, right. Then I'd just go for degree() (for points) and subset_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 dimpase

Replying to ncohen:

Ah, right. Then I'd just go for degree() (for points) and subset_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 find degree(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 follow-up: Changed 5 years ago by dimpase

+        - ``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 ; follow-up: Changed 5 years ago by ncohen

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. Then degrees(1) or degrees() would to what your degree() currently does.

1-year 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 ; follow-up: Changed 5 years ago by dimpase

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 ; follow-up: Changed 5 years ago by 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.

Can you answer the question about "set=" or "subset=" ?

Nathann

comment:30 in reply to: ↑ 29 Changed 5 years ago by dimpase

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 dimpase

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 follow-up: Changed 5 years ago by 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 set

That's all it takes.

And yes, please get rid of p= and of sets_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 follow-up: Changed 5 years ago by vdelecroix

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 (if None we assume that there is no amiguity, otherwise it must be set to True or False).
  • degrees(self, size=1): return the set of degrees of points. If size 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 dimpase

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 set

That's all it takes.

until someone decides to change the code, and adds stuff before S=...

And yes, please get rid of p= and of sets_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.

The only reason it is done so in PermutationGroup.orbitis that action can be user-defined (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, run-time 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.

Last edited 5 years ago by dimpase (previous) (diff)

comment:35 in reply to: ↑ 33 Changed 5 years ago by ncohen

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 self-explaining.

Nathann

comment:36 Changed 5 years ago by git

  • Commit changed from f82b9c41095b538a5da29ba015264c24adddc7c4 to f4f8dd6ee1c6ad08385cd1e0d00d2457cade86a4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f4f8dd6trac #17108: IncidenceStructure.degree(subset=1) and .degrees()

comment:37 follow-up: Changed 5 years ago by vdelecroix

  • 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:

527bbc1trac #17108: doc and sums

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by dimpase

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 ncohen

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 dimpase

  • Reviewers set to Vincent Delecroix, Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:41 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/17108 to 527bbc11155bc54479f53532d82ed7f774dfc829
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.