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
Division in a finite extension #19593
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ad8fa4d
Division in a finite extension
jksuom 11688d2
Merge branch 'master' into agca-extensions-division
gschintgen 8df77d1
agca-extensions: convert to python3
gschintgen 69f0b5a
agca-extensions: add tests for inverse/division
gschintgen 28d484f
agca-extensions: add public documentation
gschintgen 775921b
agca.extensions: reword note concerning Domain
gschintgen cc87228
Merge branch 'master' into agca-extensions-division
gschintgen File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this be used as a domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more a fitting comment might be that
FiniteExtension
is not a subclass ofDomain
. However, it should be possible to define aFiniteField
class that would derive fromDomain
andFiniteExtension
. It is also possible to define other wrapper subclasses ofDomain
for (some types of)FiniteExtension
. (ConsiderPolynomialRing
vs.PolyRing
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:lev
argument ofDMP
?sympy/sympy/polys/polyclasses.py
Lines 145 to 162 in a69a68c
Isn't the
old_polynomialring
somewhat deprecated? Does it matter for this application?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the lev argument is basically the number of generators:
I think it's called
lev
as short for "level" representing the fact that in the dense representation a poly inn
generators is implemented as a poly of polys inn-1
generators. The internal representation is a list of domain elements for an univariate poly and then a list of lists for bivariate etc:The
lev
argument basically tells thedmp_*
functions how many levels to recurse.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'm also hitting circular import issues (concerning
Poly
) when I try adding aPrimePowerFiniteField
class indomains/finitefield.py
that subclassesFiniteField
andFiniteExtension
. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lev
is the number of generators minus 1.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have suggested another approach: Define a new class, say,
FiniteRing
, that would do whatFiniteField
does now (or simply renameFiniteField
). InvokingFiniteField
with a non-prime argument would pass the call toFiniteRing
, presumably with a deprecation statement. ThenFiniteField
could subclass fromFiniteExtension
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.