sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 478 forks source link

fix toy_d_basis.py #6265

Closed 93749b3a-c0a4-47a7-b178-004ba08b0b97 closed 7 years ago

93749b3a-c0a4-47a7-b178-004ba08b0b97 commented 15 years ago

As discussed at #6051. Line 91 of sage/rings/polynomial/toy_d_basis.py needs to be unrandomed when this is fixed.

However, when we compute the Groebner basis of I (defined over `\ZZ`), we note that there is a certain integer in the ideal which is not 1.::

    sage: d_basis(I) # random -- waiting on upstream singular fixes at #6051
    [x + 170269749119, y + 2149906854, z + ..., 282687803443]                                

CC: @malb @tscrim @kedlaya @simon-king-jena

Component: commutative algebra

Author: Frédéric Chapoton

Branch/Commit: 39616a7

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/6265

fchapoton commented 7 years ago

Commit: 24ad1fe

fchapoton commented 7 years ago

Branch: u/chapoton/6265

fchapoton commented 7 years ago

Author: Frédéric Chapoton

fchapoton commented 7 years ago

New commits:

24ad1fetrac 6265 remove an old # random marker
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 24ad1fe to 716ac65

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

716ac65trac 6265 fixing doctest
tscrim commented 7 years ago
comment:9

It doesn't seem like the problem is fixed if you have to use ... in the doctest. So I'm not sure we should remove the comment, but I haven't looked to deep into this.

fchapoton commented 7 years ago
comment:10

Indeed, I also have some doubts. Let us check "needs_info"

The ... are there to replace huge numbers. Maybe we should keep the top few digits ?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 716ac65 to 140579b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

828443dMerge branch 'u/chapoton/6265' in 8.0.b0
140579btrac 6265 another tentative
fchapoton commented 7 years ago
comment:12

Here is another try, with green bot. I changed the term order so that the Groebner basis has much smaller coefficients. Maybe this should be tested on diverse machines.

fchapoton commented 7 years ago
comment:14

maybe this is good enough, I don't know..

tscrim commented 7 years ago
comment:15

Hmm...I'm now not so sure what the point of the doctest is. From the description above it looks more like the minimal integer that is in the ideal is the important point rather than the integers that are part of the binomial generators. I'm cc-ing Simon to see if he has a thought on this. Yet, this was last touched in 2009, so maybe we can just ... the binomial term integers and be done with it.

simon-king-jena commented 7 years ago
comment:16

Replying to @tscrim:

Hmm...I'm now not so sure what the point of the doctest is. From the description above it looks more like the minimal integer that is in the ideal is the important point rather than the integers that are part of the binomial generators. I'm cc-ing Simon to see if he has a thought on this.

I don't know anything about the theory of d-Gröbner bases. However, from the explanation in the doctest, it seems to me that the integers in the binomial generators do not matter (at least not for the test). It seems to me that the point of the example is to show that

tscrim commented 7 years ago
comment:17

Thanks Simon.

So shall we revert back to 716ac65?

fchapoton commented 7 years ago
comment:18

IMHO, the current branch should be just as good. I would rather try to let this in and see if the buildbots of Volker report any failure on apples and oranges.

tscrim commented 7 years ago
comment:19

Okay, then if you break the long line into each of the generators, you can set a positive review on my behalf.

tscrim commented 7 years ago

Reviewer: Travis Scrimshaw

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

39616a7trac 6265 break lines
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 140579b to 39616a7

fchapoton commented 7 years ago
comment:21

ok, done. Thanks. Setting to positive

vbraun commented 7 years ago

Changed branch from u/chapoton/6265 to 39616a7