Opened 6 years ago
Closed 6 years ago
#17927 closed defect (fixed)
Discarded arguments in IntegerVector
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  combinatorics  Keywords:  
Cc:  jhpalmieri, jdemeyer, aschilling, tscrim, vdelecroix, combinat, nthiery  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  4301c4c (Commits, GitHub, GitLab)  Commit:  4301c4cbdb48a00e9950292b7ce2e84a2b0388c0 
Dependencies:  Stopgaps: 
Description
As reported in #17898, IntegerVector
ignores some of its arguments
sage: list(IntegerVectors(4, length=3, min_part=1)) # a bug? [[4], [3, 1], [2, 2], [2, 1, 1], [1, 3], [1, 2, 1], [1, 1, 2], [1, 1, 1, 1]
This branch makes it raise exceptions where it should
Nathann
Change History (34)
comment:1 Changed 6 years ago by
 Branch set to public/17927
 Cc jhpalmieri jdemeyer aschilling tscrimshaw vdelecroix combinat added
 Commit set to 7dcfefbc3dc8e20eabbe1d88bd9100b2873b0582
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit changed from 7dcfefbc3dc8e20eabbe1d88bd9100b2873b0582 to f566dbf932955b91f1148d8ac2aca77e081a85c1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f566dbf  trac #17927: Discarded arguments in IntegerVector

comment:3 Changed 6 years ago by
 Cc tscrim added; tscrimshaw removed
comment:4 Changed 6 years ago by
 Commit changed from f566dbf932955b91f1148d8ac2aca77e081a85c1 to 4b5ef5365d54a0e32bf693b35440faddf8262686
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4b5ef53  trac #17927: Discarded arguments in IntegerVector

comment:5 Changed 6 years ago by
I just updated the patch. It is not simpler, as all arguments are actually forwarded to the functions, which then refuse the ones that they do not accept. This way Python raises the exceptions for us.
Nathann
comment:6 followup: ↓ 7 Changed 6 years ago by
Note that it would be cool to actually conert the input to either Python integer or Sage integers (and also make the output uniform, i.e. being list of Python integer or list of Sage integers but mixing both can lead to disaster):
sage: V = IntegerVectors(4/1, 3/1) sage: a = V.first() sage: a in V False sage: type(a[0]) <type 'sage.rings.rational.Rational'> sage: type(a[1]) <type 'int'>
Not exactly the topic of the ticket but I bumped into that issue and saw this ticket in the trac list...
Vincent
comment:7 in reply to: ↑ 6 Changed 6 years ago by
Not exactly the topic of the ticket but I bumped into that issue and saw this ticket in the trac list...
It would already be very cool if this ticket could be reviewed as it is. I wonder if anybody uses that class, but the number of inconsistencies it contains is amazing.
Nathann
comment:8 Changed 6 years ago by
Guys ?...
comment:9 followup: ↓ 10 Changed 6 years ago by
I am still waiting for an answer to 51:ticket:17979 (that ticket also makes changes to IntegerVectors
)
comment:10 in reply to: ↑ 9 Changed 6 years ago by
I am still waiting for an answer to 51:ticket:17979 (that ticket also makes changes to
IntegerVectors
)
Oh. I see.
Well, it's up to you.. But I am not sure that they will answer soon, and if they do nothing will change before #17979 gets merged too... SOooo well.
As you like.
Nathann
comment:11 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:12 Changed 6 years ago by
 Dependencies set to #18109
comment:13 Changed 6 years ago by
I don't know whether it really should depend on #18109, but at least it should be checked that there is no conflict.
comment:14 Changed 6 years ago by
 Cc nthiery added
Nicolas said that he would do it (in comment:465:ticket:17979). Could you? These bugs need to be fixed too. Thanks,
Nathann
comment:15 Changed 6 years ago by
I actually done a good chunk of work in this direction. However I very soon realized that, to avoid changing a lot of doctests temporarilly and reverting them back later on, we really want to have a class for integer vectors of a given length (with constraints). A disjoint union is not enough. Indeed quite some of the doctests are about things like checking containment which is not available with a disjoint union.
The natural thing to implement this class is to base it on
IntegerLists
(not IntegerListsLex
). Since the code base for the
later is in a state of flux, I'd rather wait a couple days and base
the work on #18109. I also want to focus on other things with people
during the Sage days here. I guess it would be doable within the next
two week.
Or we can go for a temporary solution, but it feels like just additional work for not much value: casual users won't benefit from work here before 6.7 anyway, we are not in total rush.
comment:16 Changed 6 years ago by
Nicolas,
You told me that you would rebase this ticket on top of #17979 once that it will be merged. Could you honor this promise?
We work differently on many things. I need to be able to trust people I work with. You also know that I never believed in 'ticket that will be written later', or 'code in a state of flux'. This branch was in needs_review
5 weeks ago, and has not been reviewed because we were waiting on your ticket #17979.
Please do not let my work go to waste after we waited for you.
Nathann
comment:17 followup: ↓ 18 Changed 6 years ago by
I can do that. But this really amounts in doing a crappy workaround now, breaking and "fixing" existing tests (sorry, but that's exactly what some of your changes are doing). I believe it's more efficient to shoot directly for the right thing. On other occasions you complain to me that I should not indulge myself with such temporary fixes.
I'd appreciate if I could decide myself what's the right approach in work I handle. You are not my boss.
comment:18 in reply to: ↑ 17 Changed 6 years ago by
I'd appreciate if I could decide myself what's the right approach in work I handle. You are not my boss.
You make promises that you do not keep.
comment:19 Changed 6 years ago by
(currently rebasing the code)
comment:20 Changed 6 years ago by
 Commit changed from 4b5ef5365d54a0e32bf693b35440faddf8262686 to 40c709f85d80a15c9be831f0d4b471b60c2f2107
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
40c709f  trac #17927: Discarded arguments in IntegerVector

comment:21 followup: ↓ 30 Changed 6 years ago by
 Status changed from needs_work to needs_review
Here it is. Actually, most of what the patch did had been redone in #17979
, and all that is left to do here is raise an exception when IntegerVectors(length=3)
is called.
Nathann
comment:22 Changed 6 years ago by
 Dependencies #18109 deleted
And there are no conflicts with #18109.
comment:23 followup: ↓ 24 Changed 6 years ago by
It was *your* decision to take over my work because *you* felt it was better to have this intermediate step. I was going to fullfill way more than my promise. Now you wasted my time, since I will need to rebase my work.
If you want to discourage people, that's the way to proceed.
comment:24 in reply to: ↑ 23 ; followup: ↓ 31 Changed 6 years ago by
It was *your* decision to take over my work because *you* felt it was better to have this intermediate step.
What ? To take over your work? O_o
I created this ticket when #17898 (which removed the stopgap) was reviewed (see comment:63:ticket:17898).
I was going to fullfill way more than my promise.
17 months ago you were also going to fix cartesian products (#15425). See also #18182 which was created yesterday. And there are many many things that you were going to fix 3 or 5 years ago.
Now you wasted my time, since I will need to rebase my work.
I don't know what work you are talking about. But since I created this ticket 5 weeks ago, I guess that you should base your work on top of this ticket instead of asking me to rebase mine.
Nathann
comment:25 Changed 6 years ago by
 Status changed from needs_review to needs_work
sage: [0,0,0,0] in IntegerVectors(0, 3) False sage: [3,1,2,4] in IntegerVectors(None, 3) # oops True
comment:26 Changed 6 years ago by
 Commit changed from 40c709f85d80a15c9be831f0d4b471b60c2f2107 to 4301c4cbdb48a00e9950292b7ce2e84a2b0388c0
Branch pushed to git repo; I updated commit sha1. New commits:
4301c4c  trac #17927: n=None and k!=None not supported

comment:27 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:28 Changed 6 years ago by
This achieves what the ticket claims, and it's better to have an error than something possibly wrong. This will require undoing some stuff once the real fix will come, but if that makes you happy to have this intermediate step, I can live with that. Positive review.
comment:29 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:30 in reply to: ↑ 21 Changed 6 years ago by
comment:31 in reply to: ↑ 24 Changed 6 years ago by
Replying to ncohen:
It was *your* decision to take over my work because *you* felt it was better to have this intermediate step.
What ? To take over your work?
O_o
By "my work" here, I meant "rebasing your branch on top of #17979".
17 months ago you were also going to fix cartesian products (#15425).
This is a long run task. More than half of the items are already achieved.
See also #18182 which was created yesterday.
#18182 is not a fix. It's a new feature. Beside I am actively helping out Daniel who is working on it here at Sage Days 67.
When contributing infrastructure one certainly is responsible for making sure that the infrastructure is robust, and that the basic applications are available. But there is no reason why one should implement each and every application of the infrastructure.
And there are many many things that you were going to fix 3 or 5 years ago.
And I did many of them, and in particular I believe I did all for which I had set a specific timeline. And in the mean time I did a lot of infrastructure, refactoring, and finalization work, prioritized according to the needs of many people.
Now, and again, you are not my boss to judge on how I prioritize on the many tasks that needs to be done.
I don't know what work you are talking about.
3 hours of hard work on a proper cleanup of integer vectors (currently stashed out), that I now need to rebase when it would have been more productive to do all at once.
Anyway, done with this ticket. Bye.
comment:33 Changed 6 years ago by
 Reviewers set to Nicolas M. Thiéry
 Status changed from needs_work to positive_review
comment:34 Changed 6 years ago by
 Branch changed from public/17927 to 4301c4cbdb48a00e9950292b7ce2e84a2b0388c0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #17927: Discarded arguments in IntegerVector