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: sage-6.4
Component: finite rings Keywords:
Cc: jdemeyer Merged in:
Authors: Jean-Pierre 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 jpflori

  • Branch set to u/jpflori/ticket/17165
  • Commit set to 0781ae2d0d59656357446a057830272924b73743
  • Status changed from new to needs_review

Last 10 new commits:

113a8ddMerge remote-tracking branch 'trac/develop' into ticket/15015
da483e1Correct MPIR spkg-install scripts for non-exisiting patches.
1607b8fMerge remote-tracking branch 'origin/develop' into ticket/15015
7725122Cosmetic changes
6957f17Merge remote-tracking branch 'origin/develop' into ticket/15015
aca71cbUpgrade to mpir-2.7.0-alpha12
67babebFix doctests due to changed xgcd results
8c7fbbdRe-enable "not tested" test from #4357
08fdc35Merge remote-tracking branch 'trac/u/jdemeyer/ticket/15015' into mpir
0781ae2Refactor some finite fields elements methods.

comment:2 Changed 5 years ago by git

  • Commit changed from 0781ae2d0d59656357446a057830272924b73743 to 70df9241921adad3f7412dc4218e1772923be10e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

70df924Refactor some methods for finite fields elements.

comment:3 Changed 5 years ago by jdemeyer

  • Dependencies set to #16428

comment:4 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Can you please not add # commented out code

comment:5 Changed 5 years ago by jdemeyer

I would prefer not to add the rational_reconstruction() method since FiniteField_prime_modn inherits from IntegerModRing_generic which has the method.

comment:6 follow-up: Changed 5 years ago by jpflori

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 from IntegerModRing_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 jdemeyer

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 from IntegerModRing_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 follow-up: Changed 5 years ago by jdemeyer

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 ; follow-up: Changed 5 years ago by jpflori

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 jdemeyer

Replying to jpflori:

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?

In def rational_reconstruction

comment:11 Changed 5 years ago by jpflori

Ah, I didn't read correctly your previous comment. Sorry for my stupid question.

comment:12 Changed 5 years ago by git

  • Commit changed from 70df9241921adad3f7412dc4218e1772923be10e to 6ffc64d38896bdbe238c36ca4799833525264f6a

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

6ffc64dCleanup unused code and move doctests for finite field elements.

comment:13 Changed 5 years ago by jdemeyer

inhereted -> inherited

comment:14 Changed 5 years ago by jpflori

Yes, I just saw that, fix on its way...

comment:15 Changed 5 years ago by jpflori

Some doctest output also changed.

comment:16 Changed 5 years ago by git

  • Commit changed from 6ffc64d38896bdbe238c36ca4799833525264f6a to 78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0

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

b7660edTypo in moved doctest.
96288bbMerge back changes from ticket 16930.
78fe0bfNew code produce different square root choices for FF elements.

comment:17 Changed 5 years ago by jpflori

I only have a newline issue in a doctest now...

comment:18 Changed 5 years ago by jdemeyer

raise ValueError, "must be a perfect square"

should be

raise ValueError("must be a perfect square")

comment:19 Changed 5 years ago by git

  • Commit changed from 78fe0bff4a8164ee09bb9f91ecf6ec2cea53cda0 to e00f7540fae509cb4aab82189f30535c97d4a4a1

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

e00f754Use new style Python error formatting.

comment:20 Changed 5 years ago by jpflori

Any idea of where the newline comes from?

comment:21 Changed 5 years ago by git

  • Commit changed from e00f7540fae509cb4aab82189f30535c97d4a4a1 to dad808d6234b0b4152c17a793af41e294652883b

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

dad808dTypo in error text... and update three other errors syntax.

comment:22 Changed 5 years ago by jpflori

  • 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 jdemeyer

All tests pass, but I wait for final review until next beta comes out.

comment:24 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:25 Changed 5 years ago by vbraun

  • Branch changed from u/jpflori/ticket/17165 to dad808d6234b0b4152c17a793af41e294652883b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.