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

Priority:  major  Milestone:  sage6.4 
Component:  finite rings  Keywords:  
Cc:  jdemeyer  Merged in:  
Authors:  JeanPierre Flori  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  dad808d (Commits)  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 5 years ago by
 Branch set to u/jpflori/ticket/17165
 Commit set to 0781ae2d0d59656357446a057830272924b73743
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Commit changed from 0781ae2d0d59656357446a057830272924b73743 to 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 5 years ago by
 Dependencies set to #16428
comment:4 Changed 5 years ago by
 Status changed from needs_review to needs_work
Can you please not add # commented out code
comment:5 Changed 5 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 5 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 in reply to: ↑ 6 Changed 5 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 5 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 in reply to: ↑ 8 ; followup: ↓ 10 Changed 5 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 in reply to: ↑ 9 Changed 5 years ago by
comment:11 Changed 5 years ago by
Ah, I didn't read correctly your previous comment. Sorry for my stupid question.
comment:12 Changed 5 years ago by
 Commit changed from 70df9241921adad3f7412dc4218e1772923be10e to 6ffc64d38896bdbe238c36ca4799833525264f6a
Branch pushed to git repo; I updated commit sha1. New commits:
6ffc64d  Cleanup unused code and move doctests for finite field elements.

comment:13 Changed 5 years ago by
inhereted > inherited
comment:14 Changed 5 years ago by
Yes, I just saw that, fix on its way...
comment:15 Changed 5 years ago by
Some doctest output also changed.
comment:16 Changed 5 years ago by
 Commit changed from 6ffc64d38896bdbe238c36ca4799833525264f6a to 78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0
comment:17 Changed 5 years ago by
I only have a newline issue in a doctest now...
comment:18 Changed 5 years ago by
raise ValueError, "must be a perfect square"
should be
raise ValueError("must be a perfect square")
comment:19 Changed 5 years ago by
 Commit changed from 78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0 to e00f7540fae509cb4aab82189f30535c97d4a4a1
Branch pushed to git repo; I updated commit sha1. New commits:
e00f754  Use new style Python error formatting.

comment:20 Changed 5 years ago by
Any idea of where the newline comes from?
comment:21 Changed 5 years ago by
 Commit changed from e00f7540fae509cb4aab82189f30535c97d4a4a1 to 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 5 years ago by
 Status changed from needs_work to needs_review
Ok, I forgot the final dot. What a waste of time.
comment:23 Changed 5 years ago by
All tests pass, but I wait for final review until next beta comes out.
comment:24 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:25 Changed 5 years ago by
 Branch changed from u/jpflori/ticket/17165 to dad808d6234b0b4152c17a793af41e294652883b
 Resolution set to fixed
 Status changed from positive_review to 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.