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 'AlgebraicExtensionNC' operation. #1665

Merged
merged 1 commit into from
Sep 10, 2017

Conversation

frankluebeck
Copy link
Member

  • provides a variant of 'AlgebraicExtension' that does not check
    the input polynomial for irreducibility
  • documents the new operation
  • also document the optional argument of a string for printing the
    generator of the extension
  • added a test file

- provides a variant of 'AlgebraicExtension' that does not check
  the input polynomial for irreducibility
- documents the new operation
- also document the optional argument of a string for printing the
  generator of the extension
- added a test file
Copy link
Member

@markuspf markuspf left a comment

Choose a reason for hiding this comment

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

This looks good to me apart from some minor formatting suggestions (which you might, or might not want to use).

[IsField,IsUnivariatePolynomial],0,DoAlgebraicExt);
[IsField,IsUnivariatePolynomial],0,
function(k,f) return DoAlgebraicExt(k,f,true);
end);
Copy link
Member

Choose a reason for hiding this comment

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

You could use {k,f} -> DoAlgebraicExt(k,f,true) here, which is slightly more concise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware of this new syntax. But I prefer to have code which is also readable with older GAP versions which I'm still using.

InstallMethod(AlgebraicExtensionNC,"generic",true,
[IsField,IsUnivariatePolynomial],0,
function(k,f) return DoAlgebraicExt(k,f,false);
end);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

[IsField,IsUnivariatePolynomial,IsString],0,
function(k,f,nam) return DoAlgebraicExt(k,f,nam,true);
end);

Copy link
Member

Choose a reason for hiding this comment

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

{k,f,nam} -> DoAlgebraicExt(k,f,nam,true)


RedispatchOnCondition(AlgebraicExtension,true,[IsField,IsRationalFunction],
[IsField,IsUnivariatePolynomial],0);

RedispatchOnCondition(AlgebraicExtensionNC,true,[IsField,IsRationalFunction],
[IsField,IsUnivariatePolynomial],0);

Copy link
Member

Choose a reason for hiding this comment

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

Could you format this in the same way as below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what you mean here. I have just copied the existing lines and added the NC.

Copy link
Member

Choose a reason for hiding this comment

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

I mean I like it better if it's formatted like this for reasons of line lengths (as it is done in lines 243 ff for AlgebraicExtension(NC))

RedispatchOnCondition(AlgebraicExtensionNC,true,
    [IsField,IsRationalFunction],
    [IsField,IsUnivariatePolynomial],0);

@olexandr-konovalov olexandr-konovalov added the gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall label Sep 5, 2017
@fingolfin fingolfin merged commit 0466095 into gap-system:master Sep 10, 2017
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 22, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants