Opened 5 years ago

Closed 5 years ago

#16727 closed enhancement (fixed)

IncidenceStructure.__contains__

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorial designs Keywords:
Cc: vdelecroix, knsam, dimpase Merged in:
Authors: Nathann Cohen Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: c69307c (Commits) Commit: c69307c42231458ba26b60b2259316ee38b4fa61
Dependencies: Stopgaps:

Description

Right now it is obtained through .__iter__, and it does not do the job

sage: [1,2,3,4] in IncidenceStructure([[1,2,3,4]])
True
sage: [4,3,2,1] in IncidenceStructure([[1,2,3,4]])
False

Change History (18)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16727
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to 1dd244ac51e990c6f7bafad7bb846a360e0b4d9c

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

1dd244atrac #16727: IncidenceStructure.__contains__

comment:3 Changed 5 years ago by dimpase

at least some tests should use some "real" incidence structures, not just 1-element examples...

comment:4 follow-up: Changed 5 years ago by git

  • Commit changed from 1dd244ac51e990c6f7bafad7bb846a360e0b4d9c to e7ef8c0e2b5f5ef36112428094faa8de2a32316c

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

e7ef8c0trac #16727: Moloch! Time Eater!

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

Replying to git:

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

e7ef8c0trac #16727: Moloch! Time Eater!

Well, test on non-degenerate examples, please! Your favourite STS on 15 points will do for sure...

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by ncohen

  • Branch changed from u/ncohen/16727 to public/16727

Well, test on non-degenerate examples, please! Your favourite STS on 15 points will do for sure...

Hey Dima I have no idea what doctest you want me to write but I am not going to lose 10 minutes per try until I find out. Add a commit to the branch if you don't like my doctests.

Nathann

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

Replying to ncohen:

Well, test on non-degenerate examples, please! Your favourite STS on 15 points will do for sure...

Hey Dima I have no idea what doctest you want me to write but I am not going to lose 10 minutes per try until I find out. Add a commit to the branch if you don't like my doctests.

A degenerate input is a corner case: one-element hypergraphs and complete uniform hypergraphs are surely degenerate inputs. Such tests don't stress much of the implementation, as you can imagine. Test on, say, ProjectiveGeometryDesign(3,1,GF(2)) as well.

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by ncohen

A degenerate input is a corner case: one-element hypergraphs and complete uniform hypergraphs are surely degenerate inputs. Such tests don't stress much of the implementation, as you can imagine.

Dima, are you aware that the only thing we are testing here is whether an element belongs to a LIST ? Python does not care at all what the elements are, it is all pointers as far as it is concerned.

Test on, say, ProjectiveGeometryDesign(3,1,GF(2)) as well.

Add your commit.

Nathann

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

Replying to ncohen:

A degenerate input is a corner case: one-element hypergraphs and complete uniform hypergraphs are surely degenerate inputs. Such tests don't stress much of the implementation, as you can imagine.

Dima, are you aware that the only thing we are testing here is whether an element belongs to a LIST ?

Good doctests should test the function as if it were a blackbox. The underlying implementation might change, might become more sophisticated, and the doctests should catch bugs in such future changes. Unless you prefer bug-driven development model to the test-driven one ;-)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by ncohen

Good doctests should test the function as if it were a blackbox.

Religion does not work on me.

Unless you prefer bug-driven development model to the test-driven one ;-)

I am not against doctests, I am against your spending my time on this. I believe that this function is sufficiently doctested, and I already added tests because you asked. If they don't satisfy you for some reason, I do not mind it but you will have to write your own and add it to the branch.

Nathann

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

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

Replying to ncohen:

Good doctests should test the function as if it were a blackbox.

Religion does not work on me.

It has nothing to do with religion, this is just common sense, sorry.

Unless you prefer bug-driven development model to the test-driven one ;-)

I am not against doctests, I am against your spending my time on this. I believe that this function is sufficiently doctested, and I already added tests because you asked. If they don't satisfy you for some reason, I do not mind it but you will have to write your own and add it to the branch.

I believe that I explained sufficiently clear that testing only corner cases does not work, yet you added another corner case test, for a complete uniform hypergraph is a corner case, AFAIK.

And I have elected to be a reviewer, sorry. It is my job to decide what a sufficient doctest is, not yours. Well, if you cannot add the doctests I ask for, I can surely do this too, only then you will be spending my time on this, not the other way around.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by ncohen

Yo !

I believe that I explained sufficiently clear that testing only corner cases does not work, yet you added another corner case test, for a complete uniform hypergraph is a corner case, AFAIK.

A uniform hypergraph is a "corner case" in hypergraph theory. Not as an example of Python list. I mixed integers and strings, and order. That's how I tested non-corner cases that made sense from the point of view of what this function does. It is also a doctest of the bug I reported in this ticket's description.

And I have elected to be a reviewer, sorry. It is my job to decide what a sufficient doctest is, not yours.

Indeed, but when you see that you cannot explain what you want without making lose the other guy's time because he apparently does not get it, you can save everybody by writing the 4 lines yourself.

Nathann

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

Replying to ncohen:

Yo !

I believe that I explained sufficiently clear that testing only corner cases does not work, yet you added another corner case test, for a complete uniform hypergraph is a corner case, AFAIK.

A uniform hypergraph is a "corner case" in hypergraph theory. Not as an example of Python list. I mixed integers and strings, and order. That's how I tested non-corner cases that made sense from the point of view of what this function does. It is also a doctest of the bug I reported in this ticket's description.

And I have elected to be a reviewer, sorry. It is my job to decide what a sufficient doctest is, not yours.

Indeed, but when you see that you cannot explain what you want without making lose the other guy's time because he apparently does not get it, you can save everybody by writing the 4 lines yourself.

OK, will do...

Nathann

comment:14 in reply to: ↑ 13 Changed 5 years ago by dimpase

Replying to dimpase:

OK, will do...

after I'm done building 6.3.beta7, that is...

comment:15 Changed 5 years ago by git

  • Commit changed from e7ef8c0e2b5f5ef36112428094faa8de2a32316c to c69307c42231458ba26b60b2259316ee38b4fa61

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

c69307cadded a non-generic examples, and some minor changes

comment:16 Changed 5 years ago by ncohen

Yoooooooooooooooo !

The tests pass, so if that's okay for you... :-)

Nathann

comment:17 Changed 5 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:18 Changed 5 years ago by vbraun

  • Branch changed from public/16727 to c69307c42231458ba26b60b2259316ee38b4fa61
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.