Opened 8 years ago
Closed 8 years ago
#17165 closed enhancement (fixed)
Refactor some generic finite field code
Reported by:  JeanPierre Flori  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  finite rings  Keywords:  
Cc:  Jeroen Demeyer  Merged in:  
Authors:  JeanPierre Flori  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  dad808d (Commits, GitHub, GitLab)  Commit:  dad808d6234b0b4152c17a793af41e294652883b 
Dependencies:  #16428  Stopgaps: 
Description
There are some generic methods defined in element_ext_pari
files.
There is no reason why they shouldn't be in element_base
files.
Change History (25)
comment:1 Changed 8 years ago by
Branch:  → u/jpflori/ticket/17165 

Commit:  → 0781ae2d0d59656357446a057830272924b73743 
Status:  new → needs_review 
comment:2 Changed 8 years ago by
Commit:  0781ae2d0d59656357446a057830272924b73743 → 70df9241921adad3f7412dc4218e1772923be10e 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
70df924  Refactor some methods for finite fields elements.

comment:3 Changed 8 years ago by
Dependencies:  → #16428 

comment:4 Changed 8 years ago by
Status:  needs_review → needs_work 

Can you please not add # commented out code
comment:5 Changed 8 years ago by
I would prefer not to add the rational_reconstruction()
method since FiniteField_prime_modn
inherits from IntegerModRing_generic
which has the method.
comment:6 followup: 7 Changed 8 years ago by
I'll remove the code I moved and commented out.
I don't really get your second request (without actually thinking), so please help me, you mean:
 the
FiniteField_prime_modn
both inherits fromIntegerModRing_generic
and the base class for FF elements?
So should we completely trash out the moved rational reconstruction code?
It actually checks the parent is a prime field, so the element should be in a FiniteField_prime_modn
.
comment:7 Changed 8 years ago by
Replying to jpflori:
I don't really get your second request (without actually thinking), so please help me, you mean:
 the
FiniteField_prime_modn
both inherits fromIntegerModRing_generic
and the base class for FF elements?
yes, there is
class FiniteField_prime_modn(FiniteField_generic, integer_mod_ring.IntegerModRing_generic):
So should we completely trash out the moved rational reconstruction code?
yes, remove the code and move the doctests to src/sage/rings/finite_rings/integer_mod.pyx
comment:8 followup: 9 Changed 8 years ago by
And remove the unused doctest line
sage: from sage.rings.finite_rings.finite_field_ext_pari import FiniteField_ext_pari
comment:9 followup: 10 Changed 8 years ago by
Replying to jdemeyer:
And remove the unused doctest line
sage: from sage.rings.finite_rings.finite_field_ext_pari import FiniteField_ext_pari
Which one do you mean?
comment:10 Changed 8 years ago by
comment:11 Changed 8 years ago by
Ah, I didn't read correctly your previous comment. Sorry for my stupid question.
comment:12 Changed 8 years ago by
Commit:  70df9241921adad3f7412dc4218e1772923be10e → 6ffc64d38896bdbe238c36ca4799833525264f6a 

Branch pushed to git repo; I updated commit sha1. New commits:
6ffc64d  Cleanup unused code and move doctests for finite field elements.

comment:16 Changed 8 years ago by
Commit:  6ffc64d38896bdbe238c36ca4799833525264f6a → 78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0 

comment:18 Changed 8 years ago by
raise ValueError, "must be a perfect square"
should be
raise ValueError("must be a perfect square")
comment:19 Changed 8 years ago by
Commit:  78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0 → e00f7540fae509cb4aab82189f30535c97d4a4a1 

Branch pushed to git repo; I updated commit sha1. New commits:
e00f754  Use new style Python error formatting.

comment:21 Changed 8 years ago by
Commit:  e00f7540fae509cb4aab82189f30535c97d4a4a1 → dad808d6234b0b4152c17a793af41e294652883b 

Branch pushed to git repo; I updated commit sha1. New commits:
dad808d  Typo in error text... and update three other errors syntax.

comment:22 Changed 8 years ago by
Status:  needs_work → needs_review 

Ok, I forgot the final dot. What a waste of time.
comment:23 Changed 8 years ago by
All tests pass, but I wait for final review until next beta comes out.
comment:24 Changed 8 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_review 
comment:25 Changed 8 years ago by
Branch:  u/jpflori/ticket/17165 → dad808d6234b0b4152c17a793af41e294652883b 

Resolution:  → fixed 
Status:  positive_review → closed 
Last 10 new commits:
Merge remotetracking branch 'trac/develop' into ticket/15015
Correct MPIR spkginstall scripts for nonexisiting patches.
Merge remotetracking branch 'origin/develop' into ticket/15015
Cosmetic changes
Merge remotetracking branch 'origin/develop' into ticket/15015
Upgrade to mpir2.7.0alpha12
Fix doctests due to changed xgcd results
Reenable "not tested" test from #4357
Merge remotetracking branch 'trac/u/jdemeyer/ticket/15015' into mpir
Refactor some finite fields elements methods.