improved PolynomialSequence

malb commented 10 years ago

This now works:

sage: sr = mq.SR(1,1,2,4, gf2=True)
sage: F,s = sr.polynomial_system()
sage: I = F.ideal()
sage: I.random_element(degree=3) # new: return some funny distribution, #11850
k100*k102*x102 + k100*k102*x110 + ...

sage: I.random_element(degree=3, compute_gb=True, terms=True) # new: uniformly random up to degree 3
k100*k101*w112 + k100*k102*k110 + k100*k102*w102 + k100*k103*w111 + ...

sage: F.is_groebner() # new: PolynomialSequence.is_groebner, cf. #10856
False

sage: F.maximal_degree() # new: PolynomialSequence.maximal_degree, cf. 
2

sage: gb = F.groebner_basis(algorithm='magma') # new: this used to fail
sage: gb.is_groebner()
True

sage: gb.maximal_degree() 
1

sage: M = Sequence([f.lm() for f in F]) # new: this used to fail cf. #10680
sage: M.maximal_degree()
2

sage: F.reduced() # new: interreduced_basis() moved to Sequence
Polynomial Sequence with 60 Polynomials in 36 Variables

This ticket replaces #11850, #10856, #10680

CC: @miguelmarco

Component: commutative algebra

Keywords: sd59

Author: Martin Albrecht

Branch/Commit: 1b208b5

Reviewer: Jakob Kroeker

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

malb commented 10 years ago

Description changed:

--- 
+++ 
@@ -30,3 +30,5 @@
 sage: F.reduced() # new: interreduced_basis() moved to Sequence
 Polynomial Sequence with 60 Polynomials in 36 Variables

+ +This ticket replaces #11850, #10856, #10680

malb commented 10 years ago

Commit: fef7d3b

malb commented 10 years ago

Branch: u/malb/t16585_mpolynomial_sequence

malb commented 10 years ago

comment:2

Anyone up for reviewing this?

jdemeyer commented 10 years ago

comment:3

This obviously needs to be merged with the latest development version.

Apart from that, this ticket seems to do a lot of things (not only fixing several independent bugs, but also refactoring code), so it could be seen as a patch bomb which is a bad thing. Since this patch touches Magma-related stuff, only people who have Magma could review this.

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

Changed commit from fef7d3b to 6e9ef02

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

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

6e9ef02 Merge branch 'develop' of trac.sagemath.org:sage into u/malb/t16585_mpolynomial_sequence

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:6

First remark:

in the commit 1b7927cb54bb2943ad2ae1a43c8e3befdf6703b6 in file src/sage/rings/polynomial/multi_polynomial_ideal.py at line 4139 I think it should be if d >= 0 instead of if d > 0

Second remark:

in the commit fffdbe0c8f9a248b3436d9c846bcd7b04a036d88 you introduce option terms=True to choose maximum number of terms. I don't like it, because the option is not self-descriptive, but try to convince me.

Third remark: in 81c951fa9d428816a44dd8855d5a00ec4c3cf2b3

you change the default option 'choose_degree=False' Changing default options is dangerous since it may break existing code. Please give reasons why this change is still ok

commits 3b6c11a13eb300f0a2bf6acb56d7b9001b03e1f8 c2a8506eed5d9db0106b38d1c643f503d19ab54a 37fb67cce683927cd248b518602e81a2e33ac134

looks ok for me

I will continue the review tomorrow evening.

Jakob

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

Reviewer: Jakob Kroeker

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:7

Ok, here the missing review comments:

############################################################################## 1. "make sure we use the polynomial ring as ring not the monoid", in file ./src/sage/rings/polynomial/multi_polynomial_sequence.py

I think there is a failing example, if the input below is legal:

R.<x,y,z> = PolynomialRing(QQ)
I = Ideal(5000*x*R,y*x+5) #error
# Ring is Monoid of ideals of Multivariate Polynomial Ring in x, y, z over Rational Field =>error:
# R must be a commutative ring

update: something is fishy with the next failing example: a. what was K ? b. how to reproduce "not a constant polynomial" error

second failing example (different error):

R.<x,y,z> = PolynomialRing(QQ)
I = Ideal(5000*x*K+y*x,y*x+5) 
# TypeError: not a constant polynomial

####################################################### 2. in commit 9ee750ef52d6d6ba43254a07ffbf9cea1160315f snippet:

+ try:
+ c = K.characteristic()
+ except NotImplementedError:
+ c = 0

Why is c set to zero? At least this should be documented. Is it not possible to have a different characteristic in case of an NotImplementedError? Do you have an example which hits this issue?

#################################################################### 3. refactored (lib)Singular GB code, in commit e60f83a9e1584a899e8c79171ce177474b9fed06 in src/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pyx , line 114, ff.sage_ideal_to_singular_ideal Is there a corresponding test for a conversion from "or a list of generators"? If not, could you add one?

Same commit, in file 'src/sage/rings/polynomial/polynomial_singular_interface.py renaming singular_default to singular; why? At other places there are singular_default usages. Is there a difference?

#################################################################### 4. Same commit, It seems that reduce examples show no examples for explicitly reducing leading coefficient, Could you add an example or a test which hits "ret.append(f.lc()**(-1)*f)"?

#################################################################### 5. Sequence has no 'reduced' function Is that ok? Example:

R.<x> = PolynomialRing(QQ)
F = Sequence([x,x+1])
F.reduced() # fails

#################################################################### 6. commit a88b44703751c13e451252ca2da671679077711e (fix Magma interface broken when moving decorators around):

Could you explain what may get wrong and add an example?

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

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`80d9973`	`add option terms=True for random element in multivariate polynomial rings + bugfix`
`81cf80a`	`PolynomialSequence.is_groebner`
`e1acc1c`	`MPolynomialIdeal.random_element`
`ac26dd8`	`BooleanPolynomialRing.random_element(terms=True)`
`5d6f994`	`don't use choose_degree=True as it biases the distribution`
`3f47a64`	`fix Magma interface broken when moving decorators around`
`ae4110c`	`moved interreduced_basis to sequence also for Boolean polynomials`
`0fa7a24`	`MPolynomialIdeal.random_element bugfix`
`563baf0`	`Change terms=True to terms=Infinity`
`c024197`	`Merge branch 'develop' of trac.sagemath.org:sage into u/malb/t16585_mpolynomial_sequence`

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

Changed commit from 6e9ef02 to c024197

malb commented 9 years ago

comment:9

Hi Jakob, thanks for your review. I started addressing the issues you identified.

in file src/sage/rings/polynomial/multi_polynomial_ideal.py at line 4139 I think it should be if d >= 0 instead of if d > 0

Fixed: 0fa7a24

you introduce option terms=True to choose maximum number of terms. I don't like it, because the option is not self-descriptive, but try to convince me.

I agree, so I changed it. I changed the True to Infinity and changed the behaviour such that when requesting more terms than exist the number is silently reduced to the maximum number of available terms: 563baf0

PS: Because I am stupid, though, I rebased instead of merged. Hence, I believe you'll have to check out the code again, i.e. you can't just git pull again in your current branch. Sorry for that!

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:10

you can't just git pull again in your current branch I believe you'll have to check out the code again

Thanks for the hint! Its likely that otherwise I would get confused by the merge conflicts.

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

Changed commit from c024197 to 620322d

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

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

`667dde3`	`Merge branch 'develop' into u/malb/t16585_mpolynomial_sequence`
`dfe5ed0`	`Merge branch 'develop' into u/malb/t16585_mpolynomial_sequence`
`3d4e7f7`	`Add test for conversion to singular ideals from ideals and sequences`
`adeffda`	`Document why we can assume c = 0`
`36fce47`	`Add test for reduction of leading coefficients`
`620322d`	`Add another test for reduction of leading coefficients`

malb commented 9 years ago

comment:12

you change the default option 'choose_degree=False' Changing default options is dangerous since it may break existing code. Please give reasons why this change is still ok

It changes the behaviour of a random sampler so the output is not deterministic anyway. Also, choose_degree=Truedoes not return what I’d expect from a random sampler: something uniform if possible. Mod p the new default lives up to this expectation if we consider uniform to mean: uniform of a given degree.

"make sure we use the polynomial ring as ring not the monoid", in file ./src/sage/rings/polynomial/multi_polynomial_sequence.py

I think there is a failing example, if the input below is legal:

sage: R.<x,y,z> = PolynomialRing(QQ)
sage: I = Ideal(5000*x*R,y*x+5) #error
# Ring is Monoid of ideals of Multivariate Polynomial Ring in x, y, z over Rational Field =>error:
# R must be a commutative ring

I don’t think this is connected. I’m getting the same error with sage 6.4.1. Also, The business of monoid vs ring only occurs when PolyBoRi implements the ring.

a second failing example (different error):

sage: R.<x,y,z> = PolynomialRing(QQ)
sage: I = Ideal(5000*x*K+y*x,y*x+5) 
# TypeError: not a constant polynomial

See above.

Why is c set to zero? At least this should be documented. Is it not possible to have a different characteristic in case of an NotImplementedError?? Do you have an example which hits this issue?

I added a comment:

# We assume that our ring has characteristic zero if it does not implement a # characteristic(). For example, generic quotient rings do not have a characteristic() # method implemented. It is okay to set c = 0 here because we're only using the # characteristic to pick a more specialized implementation for c = 2.

in src/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pyx , line 114, ff.sage_ideal_to_singular_ideal Is there a corresponding test for a conversion from "or a list of generators"? If not, could you add one?

I added a test.

in file 'src/sage/rings/polynomial/polynomial_singular_interface.py renaming singular_default to singular; why? At other places there are singular_default usages. Is there a difference?

There is no need to rename singular to singular_default so I dropped it. It makes no difference either way.

It seems that reduce examples show no examples for explicitly reducing leading coefficient, Could you add an example or a test which hits "ret.append(f.lc()*(-1)f)"?

I added one.

Sequence has no 'reduced' function Is that ok? Example:

I think this is fine as Sequence is not only for polynomials but for anything. However, reduced() only seems to make sense for polynomials.

(fix Magma interface broken when moving decorators around): Could you explain what may get wrong and add an example?

Unfortunately, I forgot. I could investigate if you insist.

malb commented 9 years ago

comment:13

PS: Thanks again for your comments and sorry for taking so long to reply!

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:14

Thanks again for your comments and sorry for taking so long to reply!

You are welcome!

PS: I'm currently busy so I can't promise to look at the patch earlier than next Thursday

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:15

positive review.

I rebased the commits on recent develop branch

and ran all the tests again.

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

Changed branch from u/malb/t16585_mpolynomial_sequence to u/jakobkroeker/t16585_mpolynomial_sequence.rebased

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

Changed commit from 620322d to 68c684c

vbraun commented 9 years ago

comment:16

Breaks PDF docs:

! Package inputenc Error: Unicode char \u8:ᵢ not set up for use with LaTeX.

See the inputenc package documentation for explanation.
Type  H <return>  for immediate help.
 ...                                              

l.22080 ...element in this ideal as $r = \sum hᵢ
                                                  ·fᵢ$.
?

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

Changed commit from 68c684c to 1b208b5

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

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

1b208b5 removed unicode char from documentation

malb commented 9 years ago

comment:18

./sage --docbuild reference pdf went through without errors with Jakob's fix.

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:19

the reference builds, but building all pdf documentation fails

./sage --nodotsage --docbuild all pdf

malb commented 9 years ago

comment:20

What is the error message?

ea1d0bf8-c27a-4548-8cb7-de0b1d02441a commented 9 years ago

comment:21

What is the error message?

the failure was likely related to incremental docbuild (I didn' keep the error message)

Running 'make doc-clean' first did the trick ( the docbuild succeeds with warnings)

Can you confirm that './sage --nodotsage --docbuild all pdf' succeeds`?
Did I forget to review some other important aspects ? I must admit that there is a hint in the review checklist http://www.sagemath.org/doc/developer/trac.html?highlight=review to test if the reference manual builds or not.

malb commented 9 years ago

comment:22

I can confirm than make doc-pdf succeeds. I think this should be all :).

vbraun commented 9 years ago

Changed branch from u/jakobkroeker/t16585_mpolynomial_sequence.rebased to 1b208b5

sagemath / sage

improved PolynomialSequence #16585