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

sage.rings.function_field: Update # needs #35957

Merged
merged 20 commits into from
Jul 30, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jul 14, 2023

This is:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

I have not checked every files yet. I have the same kind of remarks in all files: some # needs sage.rings.function_field should at least be at the level of blocks, many # needs sage.rings.finite_rings statements could be at top level and may be sage.rings.function_field too.

sage: latex(w) # optional - sage.rings.finite_rings
sage: # needs sage.rings.finite_rings
sage: K.<x> = FunctionField(GF(4)); _.<Y> = K[]
sage: L.<y> = K.extension(Y^3 + x + x^3*Y)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why # needs sage.rings.function_field is not needed here

Copy link
Member Author

Choose a reason for hiding this comment

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

Right... It is needed here too. But I currently don't have a working modularized distribution with which I could test either non-prime finite fields or function field extensions. So when doctests are already marked # needs sage.rings.finite_rings, the system cannot detect that other # needs tags are needed.

I'll check if I can get sagemath-pari in working order to improve this situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Now my script was able to add this tag automatically.

sage: {x.differential(): 1} # optional - sage.rings.finite_rings sage.rings.function_field
sage: # needs sage.rings.finite_rings
sage: K.<x> = FunctionField(GF(2)); _.<Y> = K[]
sage: L.<y> = K.extension(Y^3 + x + x^3*Y) # needs sage.rings.function_field
Copy link
Contributor

Choose a reason for hiding this comment

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

but is needed here.

src/sage/rings/function_field/differential.py Outdated Show resolved Hide resolved
(8/x*z) d(x)
sage: 1/(2*z) * y.differential() # optional - sage.rings.finite_rings sage.rings.function_field
sage: 1/(2*z) * y.differential() # needs sage.rings.function_field
(8/x*z) d(x)

Copy link
Contributor

Choose a reason for hiding this comment

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

removing this empty line should allow to avoid needs sage.rings.finite_rings in the next examples, no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I have instead added a block # needs tag

src/sage/rings/function_field/differential.py Outdated Show resolved Hide resolved
sage: I = O.ideal(y) # optional - sage.rings.function_field
sage: I.divisor() # optional - sage.rings.function_field
sage: L.<y> = K.extension(Y^2 + Y + x + 1/x) # needs sage.rings.function_field
sage: O = L.maximal_order() # needs sage.rings.function_field
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, since we have # needs sage.rings.function_field, it could be at the top level of the file, no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The feature sage.rings.function_field indicates that the full function field functionality (in particular, extensions) is available.
function_field.py however is shipped with the distribution sagemath-categories to make rational function fields available, and that's what a portion of the tests here are testing.

src/sage/rings/function_field/function_field.py Outdated Show resolved Hide resolved
sage: f = Y^2 - x*Y + x^2 + 1 # irreducible but not absolutely irreducible # optional - sage.rings.finite_rings
sage: L.<y> = K.extension(f) # optional - sage.rings.finite_rings
sage: L.genus() # optional - sage.rings.finite_rings
sage: # needs sage.rings.finite_rings
Copy link
Contributor

Choose a reason for hiding this comment

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

sage.rings.finite_rings is for almost all lines of examples / test that It could be at the top of the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually because prime finite fields are always available now, I was able to remove most of these tags.

src/sage/rings/function_field/ideal.py Show resolved Hide resolved
@github-actions
Copy link

Documentation preview for this PR (built with commit 61654eb; changes) is ready! 🎉

@dcoudert
Copy link
Contributor

It seems better now. Do you have further improvements to do here ?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2023

It's ready from my side

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

This is much lighter this way.
LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2023

Thank you!

@vbraun vbraun merged commit 448341b into sagemath:develop Jul 30, 2023
12 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 30, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants