Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chinese Remainder Theorem for univariate polynomials over a field #7595

Closed
rlmill mannequin opened this issue Dec 3, 2009 · 24 comments
Closed

Chinese Remainder Theorem for univariate polynomials over a field #7595

rlmill mannequin opened this issue Dec 3, 2009 · 24 comments

Comments

@rlmill
Copy link
Mannequin

rlmill mannequin commented Dec 3, 2009

This wasn't hard to implement, since all the hard work was already done.

Component: algebra

Author: Robert Miller

Reviewer: Robert Bradshaw, John Cremona

Merged: sage-4.3.1.alpha2

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

@rlmill rlmill mannequin added this to the sage-4.3.1 milestone Dec 3, 2009
@rlmill rlmill mannequin added c: algebra labels Dec 3, 2009
@rlmill rlmill mannequin assigned aghitza Dec 3, 2009
@rlmill rlmill mannequin changed the title Chinese Remainder Theorem for polynomials over a field Chinese Remainder Theorem for univariate polynomials over a field Dec 3, 2009
@rlmill rlmill mannequin added the s: needs review label Dec 3, 2009
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 4, 2009

comment:2

Hello !! This is clearly not my field but I dare intrude : I see you are using functions "crt" and "CRT_list", and I wondered if you would find it sensible to rename them to chinese_remainer_theorem and chinese_remainder_theorem_list ? If this is too long, it may be possible to drop the "theorem" from the name, but the fact remains that if I had to use this function sometime ( which is not excluded, as it is a very famous result ), there is no way on earth I would have thought of trying the "crt" function if I had not seen it related to this ticket :-)

I am under the impression the english speakers would end this message with :

"Well, just my two cents" :-)

Nathann

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 4, 2009

comment:3

Replying to @nathanncohen:

... I wondered if you would find it sensible to rename them to chinese_remainer_theorem ...

This is irrelevant to this ticket. You should bring it up on sage-devel instead.

@rlmill rlmill mannequin added s: needs review and removed s: needs info labels Dec 4, 2009
@robertwb
Copy link
Contributor

robertwb commented Dec 8, 2009

comment:4

Nice.

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 15, 2009

Reviewer: Robert Bradshaw

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 15, 2009

comment:5

This patch wasn't applying, so I've rebased it. No real changes though.

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 15, 2009

rebased on 4.3.rc0

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 15, 2009

comment:6

Attachment: trac_7595.patch.gz

Just caught a doctest failure in sage/rings/arith.py. Oops!

@mwhansen
Copy link
Contributor

Work Issues: needs rebase

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 16, 2009

comment:8

Which ticket is the patch conflicting with?

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Dec 18, 2009

comment:9

I've checked, and this merges with the current rc1.

@rlmill rlmill mannequin added s: needs review and removed s: needs work labels Dec 18, 2009
@JohnCremona
Copy link
Member

comment:10

The patch is fine, applies to 4.3.rc0 and all tests pass in sage/rings.

I have some problems with the CRT* functions though.

  1. CRT_list does not check that the two lists have the same length; if the moduli list is shorter you get an IndexError, but it would be better to catch that and raise a more informative error.

  2. CRT_basis is rather silly. It calls CRT_list n times with the same moduli, which must be wasteful. It would be better to call plain CRT n times with suitable moduli (exercise for the reader).

Of course, I don't think that these issues should delay the current patch, but deserve a ticket of their own to make sure they are tided up.

@JohnCremona
Copy link
Member

Changed reviewer from Robert Bradshaw to Robert Bradshaw, John Cremona

@JohnCremona
Copy link
Member

Changed work issues from needs rebase to none

@mwhansen
Copy link
Contributor

mwhansen commented Jan 3, 2010

Merged: sage-4.3.1.alpha0

@mwhansen
Copy link
Contributor

mwhansen commented Jan 3, 2010

comment:11

I've made #7836 for this.

@mwhansen
Copy link
Contributor

mwhansen commented Jan 4, 2010

Changed merged from sage-4.3.1.alpha0 to none

@mwhansen
Copy link
Contributor

mwhansen commented Jan 4, 2010

comment:12

This causes failures in the following file sage/quadratic_forms/quadratic_form__ternary_Tornaria.py:

**********************************************************************
File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/devel/sage-main/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 476:
    sage: map(Q1.xi_rec, [-1,2,3,5])
Exception raised:
    Traceback (most recent call last):
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_18[8]>", line 1, in <module>
        map(Q1.xi_rec, [-Integer(1),Integer(2),Integer(3),Integer(5)])###line 476:
    sage: map(Q1.xi_rec, [-1,2,3,5])
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 481, in xi_rec
        return self.reciprocal().xi(p)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 456, in xi
        return kronecker_symbol(p, self.basiclemma(2))
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 385, in basiclemma
        a=self(self.basiclemmavec(M))
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 416, in basiclemmavec
        return CRT_list(vec,mod)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/rings/arith.py", line 2522, in CRT_list
        return moduli[0].parent()(v[0])
      File "parent.pyx", line 538, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:4956)
      File "coerce_maps.pyx", line 82, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3142)
      File "coerce_maps.pyx", line 77, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3040)
      File "integer.pyx", line 653, in sage.rings.integer.Integer.__init__ (sage/rings/integer.c:6803)
    TypeError: unable to coerce <type 'sage.modules.vector_integer_dense.Vector_integer_dense'> to an integer
**********************************************************************
File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/devel/sage-main/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 478:
    sage: map(Q2.xi_rec, [-1,2,3,5])
Exception raised:
    Traceback (most recent call last):
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_18[9]>", line 1, in <module>
        map(Q2.xi_rec, [-Integer(1),Integer(2),Integer(3),Integer(5)])###line 478:
    sage: map(Q2.xi_rec, [-1,2,3,5])
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 481, in xi_rec
        return self.reciprocal().xi(p)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 456, in xi
        return kronecker_symbol(p, self.basiclemma(2))
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 385, in basiclemma
        a=self(self.basiclemmavec(M))
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/quadratic_forms/quadratic_form__ternary_Tornaria.py", line 416, in basiclemmavec
        return CRT_list(vec,mod)
      File "/virtual/scratch/mhansen/release/4.3.1/alpha0/sage-4.3.1.alpha0/local/lib/python/site-packages/sage/rings/arith.py", line 2522, in CRT_list
        return moduli[0].parent()(v[0])
      File "parent.pyx", line 538, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:4956)
      File "coerce_maps.pyx", line 82, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3142)
      File "coerce_maps.pyx", line 77, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3040)
      File "integer.pyx", line 653, in sage.rings.integer.Integer.__init__ (sage/rings/integer.c:6803)
    TypeError: unable to coerce <type 'sage.modules.vector_integer_dense.Vector_integer_dense'> to an integer

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Jan 4, 2010

Attachment: trac_7595-failures.patch.gz

Should fix the issues in quadratic_form__ternary_Tornaria.py

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Jan 4, 2010

comment:13

This patch should fix the issues mentioned above. I wasn't sure whether to make a new ticket, or just post it here (never seen a "needs_work" closed ticket...)

@JohnCremona
Copy link
Member

comment:15

I changed the ticket to "needs review" and then gave it a positive review -- it works for me.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jan 7, 2010

comment:16

This ticket is rather messy. The patch trac_7595.patch is merged in Sage 4.3.1.alpha0 and it was then closed as being fixed in that version. Later on, the value in the field "Merged in:" was deleted. Now there is another patch trac_7595-failures.patch which hasn't been merged yet as of Sage 4.3.1.alpha1. And yet the ticket is declared as "fixed", but without a value for the field "Merged in:". I think the ticket's status should now be "open" not "fixed", until trac_7595-failures.patch is merged.

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Jan 7, 2010

comment:17

Replying to @sagetrac-mvngu:

The patch trac_7595.patch is merged in Sage 4.3.1.alpha0 ... Now there is another patch trac_7595-failures.patch which hasn't been merged yet as of Sage 4.3.1.alpha1.

The patch was rolled back-- neither is in Sage-4.3.1.alpha1

And yet the ticket is declared as "fixed"...

Apparently when the ticket was reopened, this did not revert. I don't know how to fix this, but the ticket is definitely open.

... until trac_7595-failures.patch is merged.

Repeat: as of sage-4.3.1.alpha1, both patches need to be merged.

@rlmill
Copy link
Mannequin Author

rlmill mannequin commented Jan 13, 2010

Merged: 4.3.1.alpha2

@rlmill rlmill mannequin removed the s: positive review label Jan 13, 2010
@rlmill rlmill mannequin closed this as completed Jan 13, 2010
@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jan 13, 2010

Changed merged from 4.3.1.alpha2 to sage-4.3.1.alpha2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants