Opened 5 years ago

Closed 4 years ago

#17927 closed defect (fixed)

Discarded arguments in IntegerVector

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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) 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 5 years ago by ncohen

  • Branch set to public/17927
  • Cc jhpalmieri jdemeyer aschilling tscrimshaw vdelecroix combinat added
  • Commit set to 7dcfefbc3dc8e20eabbe1d88bd9100b2873b0582
  • Status changed from new to needs_review

New commits:

7dcfefbtrac #17927: Discarded arguments in IntegerVector

comment:2 Changed 5 years ago by git

  • Commit changed from 7dcfefbc3dc8e20eabbe1d88bd9100b2873b0582 to f566dbf932955b91f1148d8ac2aca77e081a85c1

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

f566dbftrac #17927: Discarded arguments in IntegerVector

comment:3 Changed 5 years ago by tscrim

  • Cc tscrim added; tscrimshaw removed

comment:4 Changed 5 years ago by git

  • Commit changed from f566dbf932955b91f1148d8ac2aca77e081a85c1 to 4b5ef5365d54a0e32bf693b35440faddf8262686

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

4b5ef53trac #17927: Discarded arguments in IntegerVector

comment:5 Changed 5 years ago by ncohen

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

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 5 years ago by ncohen

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

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

comment:8 Changed 5 years ago by ncohen

Guys ?...

comment:9 follow-up: Changed 5 years ago by jdemeyer

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 5 years ago by ncohen

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 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:12 Changed 5 years ago by jdemeyer

  • Dependencies set to #18109

comment:13 Changed 5 years ago by jdemeyer

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 5 years ago by ncohen

  • 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 5 years ago by nthiery

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 5 years ago by ncohen

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

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

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

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 5 years ago by ncohen

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 5 years ago by ncohen

(currently rebasing the code)

comment:20 Changed 5 years ago by git

  • Commit changed from 4b5ef5365d54a0e32bf693b35440faddf8262686 to 40c709f85d80a15c9be831f0d4b471b60c2f2107

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

40c709ftrac #17927: Discarded arguments in IntegerVector

comment:21 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

Here it is. Actually, most of what the patch did had been re-done in #17979, and all that is left to do here is raise an exception when IntegerVectors(length=3) is called.

Nathann

Version 0, edited 5 years ago by ncohen (next)

comment:22 Changed 5 years ago by ncohen

  • Dependencies #18109 deleted

And there are no conflicts with #18109.

comment:23 follow-up: Changed 5 years ago by nthiery

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

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

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

comment:25 Changed 5 years ago by nthiery

  • 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 5 years ago by git

  • Commit changed from 40c709f85d80a15c9be831f0d4b471b60c2f2107 to 4301c4cbdb48a00e9950292b7ce2e84a2b0388c0

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

4301c4ctrac #17927: n=None and k!=None not supported

comment:27 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:28 Changed 5 years ago by nthiery

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 5 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:30 in reply to: ↑ 21 Changed 5 years ago by nthiery

Replying to ncohen:

Here it is. Actually, most of what the patch did had been re-done in #17979

Indeed, I had taken a lead advance on my promise, by merging in a good part of your work at the occasion where it was most productive for to do it.

comment:31 in reply to: ↑ 24 Changed 5 years ago by nthiery

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:32 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

reviewer name

comment:33 Changed 4 years ago by ncohen

  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_work to positive_review

comment:34 Changed 4 years ago by vbraun

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