Division in a finite extension #19593

jksuom · 2020-06-19T04:56:14Z

References to other Issues or PRs

Brief description of what is fixed or changed

Define division in a finite extension.

Other comments

Release Notes

polys
- Add division to finite extensions.

sympy-bot · 2020-06-19T04:56:22Z

✅

Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

polys
- Add division to finite extensions. (#19593 by @gschintgen and @jksuom)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

Define division in a finite extension.
#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* polys
    * Add division to finite extensions.    
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2020-06-19T06:43:37Z

Codecov Report

Merging #19593 into master will increase coverage by 11.419%.
The diff coverage is 61.290%.

@@              Coverage Diff               @@
##            master    #19593        +/-   ##
==============================================
+ Coverage   64.420%   75.839%   +11.418%     
==============================================
  Files          656       657         +1     
  Lines       170248    173512      +3264     
  Branches     40154     41820      +1666     
==============================================
+ Hits        109674    131590     +21916     
+ Misses       54537     36209     -18328     
+ Partials      6037      5713       -324

gschintgen · 2020-06-19T20:52:51Z

I checked this out locally and did a few simple computations in GF(27) and QQ[x]/(x**2-2). Nice work.
So I suppose this only needs a few tests? (And ideally a minimum of public documentation.)

I see the commit is actually from 2018. Do you intend to follow up on this?

jksuom · 2020-06-20T06:36:33Z

this only needs a few tests

That is why I didn't push it at the time. There seemed to be some interest in improving the FF class, and I was planning to add this as a part of it. But that interest faded away (and I also seemed to have more urgent tasks :).

You are welcome to add tests and documentation.

gschintgen · 2020-06-20T11:17:10Z

You are welcome to add tests and documentation.

Ok, will do. Thanks.

gschintgen · 2020-06-20T19:00:23Z

Tests have failed due to timeout:

sympy/solvers/ode/tests/test_single.py[17] ..www.........

The job exceeded the maximum time limit for jobs, and has been terminated.

https://travis-ci.org/github/sympy/sympy/jobs/700338224#L528

I'll add at least one more commit anyway, so let's see how that one goes. And hopefully Github picks up on the results...

oscarbenjamin · 2020-06-20T19:09:00Z

Tests have failed due to timeout:

That's because of #19536

gschintgen · 2020-06-21T12:07:22Z

I noticed that a nice exposition had already been contributed by @jksuom. Please review.
Here's how the docs will look:

@jksuom

The main exposition in agca.rst is the commit message of 077a9da (by @jksuom).

oscarbenjamin · 2020-06-21T21:59:45Z

sympy/polys/agca/extensions.py

+    =====
+
+    It's not possible to use a ``FiniteExtension`` as ``domain`` for the
+    :class:`~.Poly` class.


Why can't this be used as a domain?

A more a fitting comment might be that FiniteExtension is not a subclass of Domain. However, it should be possible to define a FiniteField class that would derive from Domain and FiniteExtension. It is also possible to define other wrapper subclasses of Domain for (some types of) FiniteExtension. (Consider PolynomialRing vs. PolyRing.)

A more a fitting comment might be that FiniteExtension is not a subclass of Domain. However, it should be possible to define a FiniteField class that would derive from Domain and FiniteExtension. It is also possible to define other wrapper subclasses of Domain for (some types of) FiniteExtension. (Consider PolynomialRing vs. PolyRing.)

I'll see what this entails and how this works out and at the very least improve the comment.

Finite fields were the motivation for looking into this, but in the meantime I've also had a closer look at galoistools.py and it definitely has everything that would be needed for implementing proper non-prime finite fields (including algorithms for determining irreducibility over Z_p and actually finding irreducible polynomials). So maybe that's the better way forward when it comes to implementing GF(p^n) as polys domain. At least it seems to have been the original plan.

I'm trying to work out a proof-of-concept for GF(p**d) using FiniteExtension, but I must confess that I'm struggling a bit with all the (messy/undocumented/labyrinthine) internals of polys. In particular:

what is the lev argument of DMP?

sympy/sympy/polys/polyclasses.py

Lines 145 to 162 in a69a68c

class DMP(PicklableWithSlots, CantSympify):

"""Dense Multivariate Polynomials over `K`. """

__slots__ = ('rep', 'lev', 'dom', 'ring')

def __init__(self, rep, dom, lev=None, ring=None):

if lev is not None:

if type(rep) is dict:

rep = dmp_from_dict(rep, lev, dom)

elif type(rep) is not list:

rep = dmp_ground(dom.convert(rep), lev)

else:

rep, lev = dmp_validate(rep)

self.rep = rep

self.lev = lev

self.dom = dom

self.ring = ring

I noticed the following:

In [60]: F = FiniteExtension(poly(x**3+x**2+2, modulus=5)) In [61]: type(F.ring) Out[61]: sympy.polys.domains.old_polynomialring.GlobalPolynomialRing

Isn't the old_polynomialring somewhat deprecated? Does it matter for this application?

I think the lev argument is basically the number of generators:

In [5]: Poly(x, x, domain=ZZ).rep.lev Out[5]: 0 In [6]: Poly(x, x, y, domain=ZZ).rep.lev Out[6]: 1 In [7]: Poly(x, x, y, z, domain=ZZ).rep.lev Out[7]: 2

I think it's called lev as short for "level" representing the fact that in the dense representation a poly in n generators is implemented as a poly of polys in n-1 generators. The internal representation is a list of domain elements for an univariate poly and then a list of lists for bivariate etc:

In [9]: Poly(x, x, domain=ZZ).rep.rep Out[9]: [mpz(1), mpz(0)] In [10]: Poly(x, x, y, domain=ZZ).rep.rep Out[10]: [[mpz(1)], []] In [11]: Poly(x, x, y, z, domain=ZZ).rep.rep Out[11]: [[[mpz(1)]], [[]]]

The lev argument basically tells the dmp_* functions how many levels to recurse.

I think the lev argument is basically the number of generators [...]

Thanks!

I'm also hitting circular import issues (concerning Poly) when I try adding a PrimePowerFiniteField class in domains/finitefield.py that subclasses FiniteField and FiniteExtension. That's probaby not unexpected given that logically the chain is domains -> polynomials -> extensions. Maybe a more direct implementation is less prone to such issues.

Concerning this PR I'll modify the note in the docstring and that's it.

Isn't the old_polynomialring somewhat deprecated?

It depends on the number of generators. For a small number of generators, say, 1, 2, (3,) and dense polynomials, I think that it is quite efficient. For a large number of generators, the nested list representation becomes very inefficient.

lev is the number of generators minus 1.

'm also hitting circular import issues (concerning Poly) when I try adding a PrimePowerFiniteField class in domains/finitefield.py that subclasses FiniteField and FiniteExtension.

I have suggested another approach: Define a new class, say, FiniteRing, that would do what FiniteField does now (or simply rename FiniteField). Invoking FiniteField with a non-prime argument would pass the call to FiniteRing, presumably with a deprecation statement. Then FiniteField could subclass from FiniteExtension.

presumably with a deprecation statement

I don't think that's necessary as long as the new FiniteField also accepts the current parameters for creating a finite field of prime order. There are only some 3 or 4 very minor (and non-sensical) tests in the whole testsuite that actually instantiate a ring of integers modulo a composite. There's no real use anywhere and the current broken behavior is (fortunately) not publicly documented.

I think it's best to take the discussion concerning the implementation to #9544.

gschintgen · 2020-06-22T18:02:14Z

The Travis build hit the timeout again... :-/

oscarbenjamin · 2020-06-22T20:04:17Z

I restarted the timed out tests which have now finished. Unfortunately there are no merge conflicts...

gschintgen · 2020-06-22T20:25:55Z

I restarted the timed out tests which have now finished. Unfortunately there are no merge conflicts...

Strange, locally I could now merge master to the branch without a hitch. Hmm. I'll push that merge commit. That should do.

gschintgen · 2020-06-23T08:43:26Z

And now the timeout hit again. I think this can wait till the slow tests/density split have been sorted out. I'll refrain from restarting the job to leave more Travis capacity for those more urgent issues...

gschintgen · 2020-06-24T18:25:42Z

Tests have now passed successfully.

oscarbenjamin · 2020-06-25T01:23:42Z

Looks good

oscarbenjamin · 2020-06-25T01:24:07Z

Thanks @jksuom and @gschintgen!

Division in a finite extension

ad8fa4d

jksuom mentioned this pull request Jun 19, 2020

Raise an exception for non-prime p in FiniteFIeld(p) #13886

Open

Yathartha22 added the polys label Jun 19, 2020

Merge branch 'master' into agca-extensions-division

11688d2

gschintgen added 2 commits June 20, 2020 12:27

agca-extensions: convert to python3

8df77d1

agca-extensions: add tests for inverse/division

69f0b5a

gschintgen force-pushed the agca-extensions-division branch from 26db924 to 4a005cd Compare June 21, 2020 12:04

agca-extensions: add public documentation

28d484f

The main exposition in agca.rst is the commit message of 077a9da (by @jksuom).

gschintgen force-pushed the agca-extensions-division branch from 4a005cd to 28d484f Compare June 21, 2020 12:25

oscarbenjamin reviewed Jun 21, 2020

View reviewed changes

agca.extensions: reword note concerning Domain

775921b

gschintgen mentioned this pull request Jun 22, 2020

Finite fields #9544

Open

Merge branch 'master' into agca-extensions-division

cc87228

oscarbenjamin merged commit 9e8bd21 into sympy:master Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Division in a finite extension #19593

Division in a finite extension #19593

jksuom commented Jun 19, 2020

sympy-bot commented Jun 19, 2020 •

edited

codecov bot commented Jun 19, 2020 •

edited

gschintgen commented Jun 19, 2020

jksuom commented Jun 20, 2020

gschintgen commented Jun 20, 2020

gschintgen commented Jun 20, 2020

oscarbenjamin commented Jun 20, 2020

gschintgen commented Jun 21, 2020

oscarbenjamin Jun 21, 2020

jksuom Jun 22, 2020

gschintgen Jun 22, 2020

gschintgen Jun 22, 2020

oscarbenjamin Jun 22, 2020

gschintgen Jun 22, 2020

jksuom Jun 22, 2020

jksuom Jun 22, 2020

jksuom Jun 22, 2020

gschintgen Jun 22, 2020

gschintgen commented Jun 22, 2020

oscarbenjamin commented Jun 22, 2020

gschintgen commented Jun 22, 2020

gschintgen commented Jun 23, 2020

gschintgen commented Jun 24, 2020

oscarbenjamin commented Jun 25, 2020

oscarbenjamin commented Jun 25, 2020

	class DMP(PicklableWithSlots, CantSympify):
	"""Dense Multivariate Polynomials over `K`. """

	__slots__ = ('rep', 'lev', 'dom', 'ring')

	def __init__(self, rep, dom, lev=None, ring=None):
	if lev is not None:
	if type(rep) is dict:
	rep = dmp_from_dict(rep, lev, dom)
	elif type(rep) is not list:
	rep = dmp_ground(dom.convert(rep), lev)
	else:
	rep, lev = dmp_validate(rep)

	self.rep = rep
	self.lev = lev
	self.dom = dom
	self.ring = ring

Division in a finite extension #19593

Division in a finite extension #19593

Conversation

jksuom commented Jun 19, 2020

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Jun 19, 2020 • edited

codecov bot commented Jun 19, 2020 • edited

Codecov Report

gschintgen commented Jun 19, 2020

jksuom commented Jun 20, 2020

gschintgen commented Jun 20, 2020

gschintgen commented Jun 20, 2020

oscarbenjamin commented Jun 20, 2020

gschintgen commented Jun 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschintgen commented Jun 22, 2020

oscarbenjamin commented Jun 22, 2020

gschintgen commented Jun 22, 2020

gschintgen commented Jun 23, 2020

gschintgen commented Jun 24, 2020

oscarbenjamin commented Jun 25, 2020

oscarbenjamin commented Jun 25, 2020

sympy-bot commented Jun 19, 2020 •

edited

codecov bot commented Jun 19, 2020 •

edited