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

Make rank return an Integer object #35519

Merged
merged 9 commits into from
May 22, 2023
Merged

Make rank return an Integer object #35519

merged 9 commits into from
May 22, 2023

Conversation

kryzar
Copy link
Contributor

@kryzar kryzar commented Apr 15, 2023

📚 Description

The following raises an error:

Fq = GF(3)
A = Fq['T']
K.<T> = Frac(A)
phi = DrinfeldModule(A, [T, 0, 1])
rank = phi.rank()
cat = phi.category()
psi = cat.random_object(rank)
...
TypeError: rank must be a positive integer

There are at least two possible fixes:

  1. Make rank return an Integer object.
  2. Make cat.random_object accept both Integers and ints.

I suggest the former, and provide an implementation.

📝 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

@DavidAyotte
Copy link
Member

This seems like a good fix, but I think a better fix would be to make the degree method of an Ore polynomial return an Integer object:

sage: P.<t> = OrePolynomialRing(GF(5)['T'], GF(5)['T'].frobenius_endomorphism())
sage: type(t.degree())
<class 'int'>

@kryzar
Copy link
Contributor Author

kryzar commented Apr 16, 2023

This seems like a good fix, but I think a better fix would be to make the degree method of an Ore polynomial return an Integer object:

sage: P.<t> = OrePolynomialRing(GF(5)['T'], GF(5)['T'].frobenius_endomorphism())
sage: type(t.degree())
<class 'int'>

Agreed.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 17, 2023

The docstring linter complains:

./sage/rings/function_field/drinfeld_modules/drinfeld_module.py:1178:1: RST218 Literal block expected; none found.

@xcaruso
Copy link
Contributor

xcaruso commented Apr 27, 2023

This seems like a good fix, but I think a better fix would be to make the degree method of an Ore polynomial return an Integer object:

sage: P.<t> = OrePolynomialRing(GF(5)['T'], GF(5)['T'].frobenius_endomorphism())
sage: type(t.degree())
<class 'int'>

I also agree. Antoine, are you going to implement this?

@kryzar
Copy link
Contributor Author

kryzar commented May 9, 2023

This seems like a good fix, but I think a better fix would be to make the degree method of an Ore polynomial return an Integer object:

sage: P.<t> = OrePolynomialRing(GF(5)['T'], GF(5)['T'].frobenius_endomorphism())
sage: type(t.degree())
<class 'int'>

I also agree. Antoine, are you going to implement this?

I can!

@kryzar
Copy link
Contributor Author

kryzar commented May 12, 2023

The degree of an Ore polynomial should now be an Integer. I hope I did everything that needed to be done!

You'll notice that some tests fail: they are related to multi polynomials, and are also present on the develop branch. I therefore ignored them.

@DavidAyotte
Copy link
Member

Thanks Antoine! It all looks good to me. I'll approve once the linter is fixed and the actions are done.

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 798045e

Copy link
Member

@DavidAyotte DavidAyotte left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

src/sage/rings/polynomial/ore_polynomial_element.pyx Outdated Show resolved Hide resolved
Copy link
Contributor

@xcaruso xcaruso left a comment

Choose a reason for hiding this comment

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

Looks good to me as well.

@vbraun vbraun merged commit 2a6d3eb into sagemath:develop May 22, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 22, 2023
@kryzar
Copy link
Contributor Author

kryzar commented May 23, 2023

Thank you Volker.

@kryzar kryzar deleted the bug-rank branch May 23, 2023 08:20
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.

5 participants