sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.2k stars 413 forks source link

caching of Groebner basis not always done #27328

Open dkrenn opened 5 years ago

dkrenn commented 5 years ago
            sage: R.<a,b> = QQ[]
            sage: I = R.ideal([a^2-a, b^2-b, a+b])
            sage: GB1 = I.groebner_basis(algorithm='libsingular:slimgb')
            sage: GB2 = I.groebner_basis()
            sage: GB1 is GB2

returns False, the Gröbner basis is recomputed. This is problematic, as a lot of methods (e.g. .variety) simply call .groebner_basis() and a precomputed Gröbner basis (maybe with a different algorithm than the default) is not used.

Component: algebra

Author: Daniel Krenn

Branch/Commit: u/dkrenn/cache-groebner @ 82d81c8

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

dkrenn commented 5 years ago

Branch: u/dkrenn/cache-groebner

dkrenn commented 5 years ago

Commit: 8acaaf1

dkrenn commented 5 years ago

New commits:

8acaaf1Trac #27328: use cache if possible and no algorithm given
dkrenn commented 5 years ago
comment:3

The proposed solution is now that if there is no algorithm given but something is cached, then this is taken.

nbruin commented 5 years ago
comment:4

The building of the cache is still reliant on cached_method. For instance, that means that if I.groebner_basis() is called and then I.groebner_basis(algorithm=<whatever the default is>), then the algorithm gets executed twice.

dkrenn commented 5 years ago
comment:5

Replying to @nbruin:

The building of the cache is still reliant on cached_method. For instance, that means that if I.groebner_basis() is called and then I.groebner_basis(algorithm=<whatever the default is>), then the algorithm gets executed twice.

Yes, this is indeed true. The problem is that I.groebner_basis() does not simply translate to I.groebner_basis(algorithm=<somealgorithmname>) at the beginning, but tries a couple of things which might lead to a result. Any ideas how this can be taken care of?

tscrim commented 5 years ago
comment:6

Replying to @dkrenn:

Replying to @nbruin:

The building of the cache is still reliant on cached_method. For instance, that means that if I.groebner_basis() is called and then I.groebner_basis(algorithm=<whatever the default is>), then the algorithm gets executed twice.

Yes, this is indeed true. The problem is that I.groebner_basis() does not simply translate to I.groebner_basis(algorithm=<somealgorithmname>) at the beginning, but tries a couple of things which might lead to a result. Any ideas how this can be taken care of?

The generic way around that is to instead have the default call self.groebner_basis(algorithm=<algorithmname>) with the chosen algorithm. The other thing you could do, and which might be better, is to explicitly set the cache in those cases.

embray commented 5 years ago
comment:7

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

dkrenn commented 5 years ago
comment:8

Replying to @tscrim:

[...] The problem is that I.groebner_basis() does not simply translate to I.groebner_basis(algorithm=<somealgorithmname>) at the beginning, but tries a couple of things which might lead to a result. Any ideas how this can be taken care of?

The generic way around that is to instead have the default call self.groebner_basis(algorithm=<algorithmname>) with the chosen algorithm.

This is not what we want: We do want that algorithm='' tries a couple of things until it succeeds (or fails after all).

The other thing you could do, and which might be better, is to explicitly set the cache in those cases.

You mean setting the cache for algorithm='' to be the same as for algorithm='chosen'. This could work ;)

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

Changed commit from 8acaaf1 to 36ba0ee

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

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

9d4ffa6Trac #27445: groebner basis for multi-variate polynomial ring with no variables
6afce66Trac #27445: polynomial rings over non-fields + major restructure of relevant code
df26e69Merge tag '8.7' into t/27445/gb-no-var
eaaa150Merge branch 't/27445/gb-no-var' into t/27328/cache-groebner
3234c66Trac #27328: remove ignored/incompatibel/ununsed *args in groebner basis computations
c0b2168Trac #27328: simplify parameter processing in groebner_basis
de2dc6cTrac #27328: fixup prot parameter in submethod
35f231fTrac #27328: enable "not tested" doctests as they work now
36ba0eeTrac #27328: do proper caching of groebner_basis
dkrenn commented 5 years ago
comment:10

Replying to @dkrenn:

You mean setting the cache for algorithm='' to be the same as for algorithm='chosen'. This could work ;)

Works indeed; see current implementation.

dkrenn commented 5 years ago
comment:11

Replying to @nbruin:

The building of the cache is still reliant on cached_method. For instance, that means that if I.groebner_basis() is called and then I.groebner_basis(algorithm=<whatever the default is>), then the algorithm gets executed twice.

This is now fixed as well.

dkrenn commented 5 years ago
comment:12

I've changed the code so that all issues are solved.

Moreover, I've simplifed the way arguments are processed and handled, and cleaned up of what seems to be historically grown (and sometimes not even used or incompatible or even ignored).

Please review :)

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

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

d5e11d2Trac #27328: fix doctests (sageinspect & cachefunc)
82d81c8Trac #27328: remove example which does not test what it claims to test anymore
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 36ba0ee to 82d81c8

dkrenn commented 5 years ago
comment:14

Forgot push; now updated :)

embray commented 5 years ago
comment:15

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

embray commented 4 years ago
comment:16

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago
comment:17

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

kedlaya commented 3 years ago
comment:19

It's been long enough that I'm getting a merge failure. Rebase?

mkoeppe commented 3 years ago
comment:20

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.