Skip to content
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

New options added and bugs fixed in ConwayPolynomials #1179

Merged
merged 4 commits into from
May 22, 2020

Conversation

zeromath
Copy link
Contributor

Changes:

  • Added new option Variable to conwayPolynomial. Now it supports to use user-designated variable as the polynomial variable.
  • Updated map(GaloisField,GaloisField). Now it works properly between GaloisFields defined with user-designated variable.
  • Documentation updates.

export {
"conwayPolynomial",
-- options to set the variable to use
"Variable"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Variable" is a symbol in the Core package, so just use it -- don't export a new one with the same name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Just changed!

fix := (p,n,co,a) -> sum(#co, i -> co#i * a^i)
conwayPolynomial = method()
conwayPolynomial(ZZ,ZZ) := (p,n) -> (
conwayPolynomial = method(Options=>{Variable=>getSymbol "a"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if you run ' installPackage "ConwayPolynomials" ', this change will lead to a problem -- the error message will be something like " can't determine package of symbol a ". The solution will be to let the default value be the string "a", and run getSymbol later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrite this part by following the code in galois.m2

@DanGrayson
Copy link
Member

This is great, thank you. What is your name?

Could you add some tests to ensure that everything works as expected?

Actually, we have a big pull request coming up, so it might be good to add the test after we merge it, because it moves all the test files to another directory.

@DanGrayson DanGrayson self-assigned this May 20, 2020
@zeromath
Copy link
Contributor Author

This is great, thank you. What is your name?

Could you add some tests to ensure that everything works as expected?

Actually, we have a big pull request coming up, so it might be good to add the test after we merge it, because it moves all the test files to another directory.

I'm Zhan Jiang. I was working in the FastLinAlg group during the just past workshop (virtually in Cleveland). I just added one test case.

@DanGrayson
Copy link
Member

I just remembered that the monoid ring construction function does "getSymbol" for us:


i10 : newPackage "Foo" ; export "foo" ; foo = () -> ZZ["a"] ; endPackage "Foo"

o13 = Foo

o13 : Package

i14 : foo()

o14 = ZZ[a]

o14 : PolynomialRing

i15 : baseName oo_0

o15 = a

o15 : Symbol

i16 : package oo

o16 = User

o16 : Package

@zeromath
Copy link
Contributor Author

I removed getSymbol function and everything seems to work fine.

@zeromath
Copy link
Contributor Author

zeromath commented May 21, 2020

Here is what I mean in the slack chat

i1 : R = GF(4)[x]; S = GF(16)[y]; f = map(S,R,{y})

o3 = map(S,R,{y, a})

o3 : RingMap S <--- R

The generator of the GF(4) in R is not mapped to S properly by the natural inclusion map map(GF(16),GF(4)), it's simply mapped to the generator of GF(16). This is actually a related problem (bug?), but more fundamental.

I think in ringmap.m2 there is a chunk of code checking whether the source has coefficientRing being GaloisField, trying to map the PrimitiveElement to the correct place. But it ends up using the naive "mapping to the generator" approach. Should we create an issue about this?

@DanGrayson
Copy link
Member

Ah. Fixing that would be a good improvement, but beyond the scope of your pull request. Why don't you create a new issue for that, and volunteer for it, if you like?

@DanGrayson DanGrayson added this to the version 1.16 milestone May 21, 2020
@DanGrayson DanGrayson changed the base branch from master to development May 22, 2020 15:43
@DanGrayson
Copy link
Member

Pulling now. Go ahead and make a separate issue for the other problem, and refer to #1179

@DanGrayson DanGrayson merged commit 211139a into Macaulay2:development May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants