Opened 6 years ago
Closed 6 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 6 years ago by
- Branch set to u/ncohen/16727
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit set to 1dd244ac51e990c6f7bafad7bb846a360e0b4d9c
comment:3 Changed 6 years ago by
at least some tests should use some "real" incidence structures, not just 1-element examples...
comment:4 follow-up: ↓ 5 Changed 6 years ago by
- Commit changed from 1dd244ac51e990c6f7bafad7bb846a360e0b4d9c to e7ef8c0e2b5f5ef36112428094faa8de2a32316c
Branch pushed to git repo; I updated commit sha1. New commits:
e7ef8c0 | trac #16727: Moloch! Time Eater!
|
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 6 years ago by
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 6 years ago by
- 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: ↓ 8 Changed 6 years ago by
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.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 6 years ago by
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: ↓ 10 Changed 6 years ago by
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: ↓ 11 Changed 6 years ago by
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
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 12 Changed 6 years ago by
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: ↓ 13 Changed 6 years ago by
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: ↓ 14 Changed 6 years ago by
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 6 years ago by
comment:15 Changed 6 years ago by
- Commit changed from e7ef8c0e2b5f5ef36112428094faa8de2a32316c to c69307c42231458ba26b60b2259316ee38b4fa61
Branch pushed to git repo; I updated commit sha1. New commits:
c69307c | added a non-generic examples, and some minor changes
|
comment:16 Changed 6 years ago by
Yoooooooooooooooo !
The tests pass, so if that's okay for you... :-)
Nathann
comment:17 Changed 6 years ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
comment:18 Changed 6 years ago by
- Branch changed from public/16727 to c69307c42231458ba26b60b2259316ee38b4fa61
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #16727: IncidenceStructure.__contains__