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 algorithms to compute the characteristic polynomial of the Frobenius endomorphism of a Drinfeld module #38174

Merged
merged 36 commits into from
Sep 22, 2024

Conversation

kryzar
Copy link
Contributor

@kryzar kryzar commented Jun 8, 2024

This pull request implements two new algorithms to compute the characteristic polynomial of the Frobenius endomorphism of a Drinfeld $\mathbb F_q[T]$-module over a finite field $K$. Previously, only the algorithms based on crystalline cohomology (see Musleh-Schost 2023) or on Anderson motives (see Caruso-Leudière 2023) were implemented. We propose two new algorithms:

Acknowledgement. This implementation was originally due to @xcaruso (see here), and after a private discussion, I took the liberty of creating this PR.

I also propose to change the formula computed by frobenius_norm. Before, it computed

$$(-1)^n \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n / \mathrm{deg}(p)},$$

where $K$ is the ground field, $n$ is the degree of $K$ over $\mathbb F_q$, and $p$ is the monic generator of the $\mathbb F_q[T]$-characteristic of $K$. The docstring claimed this was $(-1)^r$ times the constant coefficient of the characteristic polynomial of the Frobenius endomorphism, $r$ being the rank of the Drinfeld module. I believe this was a mistake, and instead changed the formula, following Theorem 4.2.7 of Papikian's book, to
$$(-1)^{nr - n - r} \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n / \mathrm{deg}(p)}.$$

P.S. @DavidAyotte, given that you now work in the industry, please tell us if you still want to be tagged on these Drinfeld module stuff. Same question for you @ymusleh given that you defended your thesis (congratulations!).

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

(Dédicace à ABLCCN)

Copy link

github-actions bot commented Jun 8, 2024

Documentation preview for this PR (built with commit 3dd3c04; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Comment on lines +408 to +420
Check that ``var`` inputs are taken into account for cached
characteristic polynomials::

sage: Fq = GF(2)
sage: A.<T> = Fq[]
sage: K.<z> = Fq.extension(2)
sage: phi = DrinfeldModule(A, [z, 0, 1])
sage: phi.frobenius_charpoly()
X^2 + X + T^2 + T + 1
sage: phi.frobenius_charpoly(var='Foo')
Foo^2 + Foo + T^2 + T + 1
sage: phi.frobenius_charpoly(var='Bar')
Bar^2 + Bar + T^2 + T + 1
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there a little bit too much emphasis on this optional flag and the documentation of this method is already quite lenghty. I think this could either: be rephrased into a more "software reference manual documentation" way OR put into a TEST block in order to hide it from the manual completely.

What I mean by the first suggestion is: rephrase the sentence like "Use the optional var parameter in order to change the variable name of the resulting univariate polynomial" or somthing like this. Also, I would reuse the last phi to make it a one-liner doctest.

Anyway, this is only my preference as someone who don't really like to read documentation and prefer to have the most straightforward info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this block (lines 408 to 420) are already in a TESTS:: block, no? See line 359.

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.

Two small comments.
Otherwise it looks good to me; however, my opinion is certainly biased since I am mostly the author of this code.

Comment on lines 429 to 430
if self._frobenius_charpoly is not None:
return self._frobenius_charpoly
return self._frobenius_charpoly.change_variable_name(var)
Copy link
Contributor

Choose a reason for hiding this comment

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

With these lines, you use the cached value even if the user explicitly asks for another algorithm. I think that it is not what we want.

Copy link
Contributor Author

@kryzar kryzar Aug 1, 2024

Choose a reason for hiding this comment

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

Agreed. Will also take care of it.
(Wasn't it already the case tho? I remember having to use some tricks in the past to not use the cached data, but maybe I was mistaken.)

Comment on lines 771 to 772
@cached_method
def frobenius_trace(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

In this method, you use the crystalline matrix in all cases. I think it is okay but aren't there any case where the motive matrix is faster to compute?

Copy link
Contributor Author

@kryzar kryzar Aug 1, 2024

Choose a reason for hiding this comment

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

Yup. Will take care of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any objection to making DrinfeldModuleMorphism._motive_matrix public? It's interesting in its own right and would be useful in our case.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by public? Without the initial underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, eventually, I plan to have a method motive to create the motive of a Drinfeld module or a morphism between those, and a method matrix to get the matrix of a morphism of Anderson motives.
So, if you make the method _motive_matrix public now, it means that it will need to be deprecated afterwards. Therefore, I prefer keeping it as it is.

@xcaruso
Copy link
Contributor

xcaruso commented Aug 26, 2024

Strong +1.
Honestly, I'm really very confused (and unhappy) by all the CI. There are tons of them and many of them basically randomly fail for some weird reason I generally can't understand. I asked several times for clarification but very rarely got an answer.

@xcaruso
Copy link
Contributor

xcaruso commented Aug 28, 2024

This time, for instance, I got the error for the check Build & Test / test-new (pull_request):

Run docker run --name BUILD -dit \
  docker run --name BUILD -dit \
             --mount type=bind,src=$(pwd),dst=$(pwd) \
             --workdir $(pwd) \
             localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci /bin/sh
  shell: /usr/bin/bash -e {0}
  env:
    TOX_ENV: docker-ubuntu-jammy-standard-incremental
    BUILD_IMAGE: localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci
    FROM_DOCKER_REPOSITORY: ghcr.io/sagemath/sage/
    FROM_DOCKER_TARGET: with-targets
    FROM_DOCKER_TAG: dev
    EXTRA_CONFIGURE_ARGS: --enable-fat-binary
Unable to find image 'localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci' locally
docker: Error response from daemon: manifest for localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci not found: manifest unknown: manifest unknown.
See 'docker run --help'.
Error: Process completed with exit code 125.

I assume this is not due to this PR.

With Build & Test using Conda / Conda (macos-latest, 3.11, environment) (pull_request), I got:

sage -t --random-seed=91866416175107778193950271457384847877 src/sage/modules/free_quadratic_module_integer_symmetric.py  # 1 doctest failed
sage -t --random-seed=91866416175107778193950271457384847877 src/sage/rings/number_field/galois_group.py  # 1 doctest failed [failed in baseline: unreported failure on macOS (sort order) in GaloisGroup_v2.artin_symbol, https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]
sage -t --random-seed=91866416175107778193950271457384847877 src/sage/rings/number_field/number_field.py  # 2 doctests failed [failed in baseline: unreported failure on macOS (sort order) in NumberField_absolute.[optimized_]subfields, https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]
sage -t --random-seed=91866416175107778193950271457384847877 src/sage/rings/qqbar.py  # 1 doctest failed [failed in baseline: unreported failure on macOS seen in https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]

Should I care about this?

All CI Linux incremental* basically also fail. Is it important?

In any case, it seems that the issue with the documentation is now fixed. So I give again a positive review. I'm sorry if I'm wrong but I'm completely lost.

@DavidAyotte
Copy link
Member

Why is the git commit history so ugly? It's quite hard to follow what has been done in this PR.

@xcaruso
Copy link
Contributor

xcaruso commented Aug 29, 2024

I think it's my fault but I don't know exactly what happened: at some point, I merged with the latest version of develop and got many commits from there in the history. It seems that github believes that PR is based on version 10.5.beta1, whereas it is based on version 10.5.beta2 (see the diff).

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 5, 2024
…mial of the Frobenius endomorphism of a Drinfeld module

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This pull request implements two new algorithms to compute the
characteristic polynomial of the Frobenius endomorphism of a Drinfeld
$\mathbb F_q[T]$-module over a finite field $K$. Previously, only the
algorithms based on crystalline cohomology (see [Musleh-Schost
2023](https://dl.acm.org/doi/10.1145/3597066.3597080)) or on Anderson
motives (see [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879))
were implemented. We propose two new algorithms:

- The algorithm based on central simple algebras described in Chapter 4
of [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879).

- The algorithm described by Gekeler in [Gekeler 1991](https://www.scien
cedirect.com/science/article/pii/002186939190211P).

**Acknowledgement.** This implementation was originally due to @xcaruso
(see [here](https://github.com/xcaruso/sage/blob/d2e36bd18b51c93806b7a3b
5c8261da7dc98c494/src/sage/rings/function_field/drinfeld_modules/finite_
drinfeld_module.py)), and after a private discussion, I took the liberty
of creating this PR.

I also propose to change the formula computed by `frobenius_norm`.
Before, it computed

$$(-1)^n \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n / \mathrm{deg}(p)},$$

where $K$ is the ground field, $n$ is the degree of $K$ over $\mathbb
F_q$, and $p$ is the monic generator of the $\mathbb
F_q[T]$-characteristic of $K$. The docstring claimed this was $(-1)^r$
times the constant coefficient of the characteristic polynomial of the
Frobenius endomorphism, $r$ being the rank of the Drinfeld module. I
believe this was a mistake, and instead changed the formula, following
Theorem 4.2.7 of [Papikian's
book](https://link.springer.com/book/10.1007/978-3-031-19707-9), to
$$(-1)^{nr - n - r} \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n /
\mathrm{deg}(p)}.$$

P.S. @DavidAyotte, given that you now work in the industry, please tell
us if you still want to be tagged on these Drinfeld module stuff. Same
question for you @ymusleh given that you defended your thesis
(congratulations!).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [X] I have created tests covering the changes.
- [X] I have updated the documentation and checked the documentation
preview.


<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->


(Dédicace à ABLCCN)
    
URL: sagemath#38174
Reported by: Antoine Leudière
Reviewer(s): Antoine Leudière, David Ayotte, Xavier Caruso
@tobiasdiez
Copy link
Contributor

The error in the conda workflow
sage -t --random-seed=91866416175107778193950271457384847877 src/sage/modules/free_quadratic_module_integer_symmetric.py # 1 doctest failed

at least seems to be introduced by this PR or not? Could this be fixed please? Thanks! (Maybe as a follow-up since its already in volkers release branch)

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 6, 2024
…mial of the Frobenius endomorphism of a Drinfeld module

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This pull request implements two new algorithms to compute the
characteristic polynomial of the Frobenius endomorphism of a Drinfeld
$\mathbb F_q[T]$-module over a finite field $K$. Previously, only the
algorithms based on crystalline cohomology (see [Musleh-Schost
2023](https://dl.acm.org/doi/10.1145/3597066.3597080)) or on Anderson
motives (see [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879))
were implemented. We propose two new algorithms:

- The algorithm based on central simple algebras described in Chapter 4
of [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879).

- The algorithm described by Gekeler in [Gekeler 1991](https://www.scien
cedirect.com/science/article/pii/002186939190211P).

**Acknowledgement.** This implementation was originally due to @xcaruso
(see [here](https://github.com/xcaruso/sage/blob/d2e36bd18b51c93806b7a3b
5c8261da7dc98c494/src/sage/rings/function_field/drinfeld_modules/finite_
drinfeld_module.py)), and after a private discussion, I took the liberty
of creating this PR.

I also propose to change the formula computed by `frobenius_norm`.
Before, it computed

$$(-1)^n \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n / \mathrm{deg}(p)},$$

where $K$ is the ground field, $n$ is the degree of $K$ over $\mathbb
F_q$, and $p$ is the monic generator of the $\mathbb
F_q[T]$-characteristic of $K$. The docstring claimed this was $(-1)^r$
times the constant coefficient of the characteristic polynomial of the
Frobenius endomorphism, $r$ being the rank of the Drinfeld module. I
believe this was a mistake, and instead changed the formula, following
Theorem 4.2.7 of [Papikian's
book](https://link.springer.com/book/10.1007/978-3-031-19707-9), to
$$(-1)^{nr - n - r} \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n /
\mathrm{deg}(p)}.$$

P.S. @DavidAyotte, given that you now work in the industry, please tell
us if you still want to be tagged on these Drinfeld module stuff. Same
question for you @ymusleh given that you defended your thesis
(congratulations!).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [X] I have created tests covering the changes.
- [X] I have updated the documentation and checked the documentation
preview.


<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->


(Dédicace à ABLCCN)
    
URL: sagemath#38174
Reported by: Antoine Leudière
Reviewer(s): Antoine Leudière, David Ayotte, Xavier Caruso
@kryzar
Copy link
Contributor Author

kryzar commented Sep 7, 2024

The error in the conda workflow sage -t --random-seed=91866416175107778193950271457384847877 src/sage/modules/free_quadratic_module_integer_symmetric.py # 1 doctest failed

at least seems to be introduced by this PR or not? Could this be fixed please? Thanks! (Maybe as a follow-up since its already in volkers release branch)

This bug is not even in the current version; I merged develop yesterday:

sage -t --random-seed=74903126027750647063226099060708583867 src/sage/modules/free_quadratic_module_integer_symmetric.py
    [248 tests, 3.21 s]

So no, I don't think so, and I can hardly find it realistic that Xavier or me would have touched this file.

@xcaruso
Copy link
Contributor

xcaruso commented Sep 18, 2024

Yo! All required tests pass! 🎉

@kryzar
Copy link
Contributor Author

kryzar commented Sep 18, 2024

gif

@kryzar
Copy link
Contributor Author

kryzar commented Sep 18, 2024

Ah bah non attends.

(Meaning: oh but wait but no.)

Some stuff related to coverage is failing. Oh well.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 19, 2024
…mial of the Frobenius endomorphism of a Drinfeld module

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This pull request implements two new algorithms to compute the
characteristic polynomial of the Frobenius endomorphism of a Drinfeld
$\mathbb F_q[T]$-module over a finite field $K$. Previously, only the
algorithms based on crystalline cohomology (see [Musleh-Schost
2023](https://dl.acm.org/doi/10.1145/3597066.3597080)) or on Anderson
motives (see [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879))
were implemented. We propose two new algorithms:

- The algorithm based on central simple algebras described in Chapter 4
of [Caruso-Leudière 2023](https://arxiv.org/abs/2307.02879).

- The algorithm described by Gekeler in [Gekeler 1991](https://www.scien
cedirect.com/science/article/pii/002186939190211P).

**Acknowledgement.** This implementation was originally due to @xcaruso
(see [here](https://github.com/xcaruso/sage/blob/d2e36bd18b51c93806b7a3b
5c8261da7dc98c494/src/sage/rings/function_field/drinfeld_modules/finite_
drinfeld_module.py)), and after a private discussion, I took the liberty
of creating this PR.

I also propose to change the formula computed by `frobenius_norm`.
Before, it computed

$$(-1)^n \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n / \mathrm{deg}(p)},$$

where $K$ is the ground field, $n$ is the degree of $K$ over $\mathbb
F_q$, and $p$ is the monic generator of the $\mathbb
F_q[T]$-characteristic of $K$. The docstring claimed this was $(-1)^r$
times the constant coefficient of the characteristic polynomial of the
Frobenius endomorphism, $r$ being the rank of the Drinfeld module. I
believe this was a mistake, and instead changed the formula, following
Theorem 4.2.7 of [Papikian's
book](https://link.springer.com/book/10.1007/978-3-031-19707-9), to
$$(-1)^{nr - n - r} \mathrm{N}_{K/\mathbb F_q}(\Delta) p^{n /
\mathrm{deg}(p)}.$$

P.S. @DavidAyotte, given that you now work in the industry, please tell
us if you still want to be tagged on these Drinfeld module stuff. Same
question for you @ymusleh given that you defended your thesis
(congratulations!).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [X] I have created tests covering the changes.
- [X] I have updated the documentation and checked the documentation
preview.


<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->


(Dédicace à ABLCCN)
    
URL: sagemath#38174
Reported by: Antoine Leudière
Reviewer(s): Antoine Leudière, David Ayotte, Xavier Caruso
@xcaruso
Copy link
Contributor

xcaruso commented Sep 19, 2024

The test is not required :-).
And more importantly, the error message is

Traceback (most recent call last):
...
FileExistsError: [Errno 17] File exists: '.coverage'

So the most likely is that there is a problem somewhere in the configuration of the container, but not in this PR.

@kryzar
Copy link
Contributor Author

kryzar commented Sep 20, 2024

Not all tests are required?!

@xcaruso
Copy link
Contributor

xcaruso commented Sep 20, 2024

I'm not sure. But some of them are marked Required and others are not.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 21, 2024

coverage ci will be fixed by #38687. You may ignore the ci failure here.

@kryzar
Copy link
Contributor Author

kryzar commented Sep 21, 2024

coverage ci will be fixed by #38687. You may ignore the ci failure here.

Many thanks for the heads-up!

@vbraun vbraun merged commit 5bc02a1 into sagemath:develop Sep 22, 2024
16 of 17 checks passed
@kryzar kryzar deleted the charpoly-csa branch September 22, 2024 11:06
@xcaruso
Copy link
Contributor

xcaruso commented Sep 22, 2024

🍾

@kryzar
Copy link
Contributor Author

kryzar commented Sep 22, 2024

🍾

+100

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.

6 participants