Ticket #1952 (closed defect: fixed)

Opened 10 months ago

Last modified 3 months ago

[with patch, positive review] Follow up to #1940: Ideal comparison improvements

Reported by: mabshoff Assigned to: malb
Priority: major Milestone: sage-3.1.2
Component: commutative algebra Keywords:
Cc:

Description 

[21:30] <wstein-1649> one thing about #1940...
[21:30] <wstein-1649> If you're comparing two ideals, it might make sense to use the term ordering for which
[21:30] <wstein-1649> computing groebner basis is *easy*.
[21:31] <wstein-1649> E.g., wrt one order it can be super super hard, and wrt to another, quite easy.
[21:31] <mabshoff> Yep. That sounds like a great idea.
[21:31] <mabshoff> i.e. degrevlex per default.
[21:31] <mabshoff> Ticket?
[21:31] <wstein-1649> But in malb's code for #1940 he just uses whatever term order.
[21:31] <mabshoff> Well, I think he caches the gbasis, so in some cases it might already exist.
[21:32] <wstein-1649> Also, he doesn't even seem to check that other is an ideal.
[21:32] <wstein-1649> Also, if the two ideals are in the same ring but with two different term orders,
[21:32] <wstein-1649> that __cmp__ function will get the wrong answer.
[21:32] <mabshoff> mmh.
[21:32] <mabshoff> I guess reopen, but leave the patch applied?
[21:32] <wstein-1649> I think his patch perfectly addresses the given #1940.
[21:32] <wstein-1649> But there should be another trac ticket about my points above.
[21:33] <wstein-1649> Yeah?
[21:33] <mabshoff> Yep.
[21:33] <wstein-1649> Can you open it and maybe paste this discussion in?
[21:33] <mabshoff> Already on the way :)
[21:33] <wstein-1649> cool.

Attachments

mpolynomial_ideal_refactor.patch (38.1 kB) - added by malb on 08/19/2008 07:59:40 AM.
trac_1952-2.patch (9.0 kB) - added by mhansen on 08/26/2008 04:07:01 PM.
trac_1952-tutfix.patch (1.6 kB) - added by malb on 08/27/2008 06:19:25 AM.

Change History

01/27/2008 12:52:34 PM changed by was 

cwitty: wstein-1649, what do you mean "in the same ring but with two different term orders"?  Doesn't Sage treat the term order as part of the ring?
[3:46pm] wstein-1649: yes.
[3:47pm] wstein-1649: cwitty -- what if I make two QQ[x,y]'s with two different term orders.  But then I define ideals I, J generated by (x,y) and (x,y).
[3:47pm] wstein-1649: Shouldn't I == J be true?
[3:47pm] wstein-1649: But with Malb's code, it would be false, sort of by accident.
[3:48pm] cwitty: I guess I don't really care whether I==J is true or not.
[3:48pm] wstein-1649: ?

Maybe I'm just confused. In any case, some further thought here would be a good idea. Mainly relevant is optimization.

01/27/2008 01:02:06 PM changed by was

This -- maybe (?) caused by #1940 -- is very inconsistent: 

sage: R = PolynomialRing(QQ, 'x,y,z', order='degrevlex'); R
Multivariate Polynomial Ring in x, y, z over Rational Field
sage: S = PolynomialRing(QQ, 'x,y,z', order='invlex'); S
Multivariate Polynomial Ring in x, y, z over Rational Field
sage: I = R.ideal([R.0,R.1])
sage: J = S.ideal([S.0,S.1])
sage: I == J
True
sage: cmp(I,J)
1
sage: I.__cmp__(J)
1

01/27/2008 01:08:54 PM changed by mabshoff

It is cause by #1940: Compare the same session in 2.10 vanilla: 

----------------------------------------------------------------------
| SAGE Version 2.10, Release Date: 2008-01-18                        |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------

sage: R = PolynomialRing(QQ, 'x,y,z', order='degrevlex'); R
Multivariate Polynomial Ring in x, y, z over Rational Field
sage: S = PolynomialRing(QQ, 'x,y,z', order='invlex'); S
Multivariate Polynomial Ring in x, y, z over Rational Field
sage: I = R.ideal([R.0,R.1])
sage: J = S.ideal([S.0,S.1])
sage: I==J
True
sage: cmp(I,J)
0
sage: I.__cmp__(J)
0
sage:

Cheers,

Michael

01/27/2008 02:06:38 PM changed by mabshoff

#1940 also seems to have broken the toy_buchberger doctest: 

sage -t  devel/sage-main/sage/rings/polynomial/toy_buchberger.py
**********************************************************************
File "toy_buchberger.py", line 60:
    sage: I = sage.rings.ideal.Katsura(P)
Expected:
    // sage334              [0]  ideal, 3 generator(s)
Got:
    // sage310              [0]  ideal, 3 generator(s)
**********************************************************************
1 items had failures:
   1 of  16 in __main__.example_0
***Test Failed*** 1 failures.
For whitespace errors, see the file .doctest_toy_buchberger.py
         [4.1 s]
exit code: 256

----------------------------------------------------------------------
The following tests failed:

Cheers,

Michael

01/27/2008 02:19:06 PM changed by was

In 

    // sage310              [0]  ideal, 3 generator(s)

the variable name just depends on how many times Singular has been called, since adding any doctest that involves Singular's pexpect interface in will break everything! It's stupid-ish to even list it. It would be much better to change the above to 

    // sage...             [0]  ideal, 3 generator(s)

01/27/2008 02:44:07 PM changed by mabshoff

William: Yep, your fix is the obvious correct one. I fixed it in my repo and attached the patch here.

01/27/2008 03:17:03 PM changed by was

  • summary changed from Follow up to #1940: Ideal comparison improvements to [with patch; positive review] Follow up to #1940: Ideal comparison improvements.

01/29/2008 04:45:13 AM changed by mabshoff

  • summary changed from [with patch; positive review] Follow up to #1940: Ideal comparison improvements to Follow up to #1940: Ideal comparison improvements.

Since the doctest patch has been merged change the Summary line for now.

Cheers,

Michael

08/19/2008 07:59:40 AM changed by malb

  • attachment mpolynomial_ideal_refactor.patch added.

08/19/2008 08:00:37 AM changed by malb

  • summary changed from Follow up to #1940: Ideal comparison improvements to [with patch, needs review] Follow up to #1940: Ideal comparison improvements.

The attached patch fixes that issue and also refactors caching of Gröbner bases.

08/24/2008 05:23:29 AM changed by malb

was, can I ask you to review my patch?

08/26/2008 04:07:01 PM changed by mhansen

  • attachment trac_1952-2.patch added.

08/26/2008 04:07:55 PM changed by mhansen

Positive review for malb's changes. Now, my latest patch needs to be looked at.

08/26/2008 05:22:07 PM changed by mabshoff

There is some slight trouble in tut.tex with both patches applied (I deleted the one line doctest fix I added a while ago): 

sage -t -long devel/doc/tut/tut.tex
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.1.2.alpha1/tmp/tut.py", line 2181:
    : D.irreducible_components()
Expected:
    [
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined
    by:
      x^3 + y^3 - 1,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined
    by:
      x^2 + y^2 - 1
    ]
Got:
    [
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined by:
      x^2 + y^2 - 1,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined by:
      x^3 + y^3 - 1
    ]
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.1.2.alpha1/tmp/tut.py", line 2197:
    : V.irreducible_components()
Expected:
    [
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined
    by:
      x + y + 2
      2*y^2 + 4*y + 3,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined
    by:
      y - 1
      x,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined
    by:
      y
      x - 1
    ]
Got:
    [
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined by:
      y
      x - 1,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined by:
      y - 1
      x,
    Closed subscheme of Affine Space of dimension 2 over Rational Field defined by:
      x + y + 2
      2*y^2 + 4*y + 3
    ]
**********************************************************************

08/27/2008 06:19:25 AM changed by malb

  • attachment trac_1952-tutfix.patch added.

08/27/2008 01:10:09 PM changed by malb

  • summary changed from [with patch, needs review] Follow up to #1940: Ideal comparison improvements to [with patch, positive review] Follow up to #1940: Ideal comparison improvements.

Apply all three patches in order (last one goes to doc repo).

08/27/2008 04:11:36 PM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged all three patches in Sage 3.1.2.alpha2