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

DrinfeldModule is_isomorphic method does the opposite of what it says for rational isomorphisms #38276

Open
2 tasks done
kryzar opened this issue Jun 25, 2024 · 4 comments · May be fixed by #38303
Open
2 tasks done

DrinfeldModule is_isomorphic method does the opposite of what it says for rational isomorphisms #38276

kryzar opened this issue Jun 25, 2024 · 4 comments · May be fixed by #38303
Labels

Comments

@kryzar
Copy link
Contributor

kryzar commented Jun 25, 2024

The documentation of the is_isomorphic method of DrnifeldModule is erroneous. On line 1316 and after it is said that by default, the method looks for an isomorphism defined on the algebraic closure; the keyword absolutely would be True if and only if the isomorphism is defined on the ground field.

        - ``absolutely`` -- a boolean (default: ``False``); if ``True``,
          check the existence of an isomorphism defined on the base
          field; if ``False``, check over an algebraic closure.

Indeed, the keyword is set to False by default (line 1309 and after):

    def is_isomorphic(self, other, absolutely=False):

But the opposite of that is coded (line
1440 and after):

        # Solve the equation u^e = ue
        # - when absolutely=True, over the algebraic closure (then a
        #   solution always exists)
        # - when absolutely=False, over the ground field.
        if absolutely:
            return True
        else:
            ue = ue.backend(force=True)
            try:
                _ = ue.nth_root(e)
            except ValueError:
                return False
            except (AttributeError, NotImplementedError):
                raise NotImplementedError(f"cannot solve the equation u^{e} == {ue}")
            return True

This must be fixed. I'm thinking that by default, the isomorphism should be
looked for in the whole algebraic closure, to be consistent with the
j_invariant method:

sage: Fq = GF(2)
sage: A.<T> = Fq[]
sage: K.<z> = Fq.extension(2)
sage: phi = DrinfeldModule(A, [z, 0, 1])
sage: psi = DrinfeldModule(A, [z, 0, z])
sage: phi.j_invariant()
0
sage: psi.j_invariant()
0
sage: phi.is_isomorphic(psi)
False
sage: phi.is_isomorphic(psi, absolutely=False)
False
sage: phi.is_isomorphic(psi, absolutely=True)
True

Maybe a clearer name for the keyword could be picked, e.g. algebraic_closure (although I do not have a strong opinion on this).


@xcaruso @DavidAyotte @ymusleh

Environment

- **OS**: Arch Linux x86_64 6.9.5-arch1-1 
- **Sage Version**: SageMath version 10.3, Release Date: 2024-03-19

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@kryzar kryzar added the t: bug label Jun 25, 2024
@xcaruso
Copy link
Contributor

xcaruso commented Jun 25, 2024

I agree that the documentation needs to be fixed (True and False should be swapped).

However, I believe that absolutely is a correct wording: very often "absolutely something" means "something over the algebraic closure" (e.g. absolutely irreducible...). In algebraic geometry, we also use the word "geometrically" but I think it is not adapted here.
Also, I prefer having absolutely=False as default. Mostly because usually when we say that X and Y are isomorphic, it is definitely not implicitly assumed that we work over an algebraic closure.

@kryzar
Copy link
Contributor Author

kryzar commented Jun 27, 2024

I'm fine with that (or the other options, for that matter).
@DavidAyotte @ymusleh: anything to say?

@DavidAyotte
Copy link
Member

I agree with what Xavier said!

@kryzar
Copy link
Contributor Author

kryzar commented Jun 27, 2024

Ok, I'll try to do that in the next few days. If somebody wants to do it, feel free to.

@kryzar kryzar linked a pull request Jun 28, 2024 that will close this issue
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this issue Jul 20, 2024
… flag

    
<!-- ^ 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". -->

Fixes sagemath#38276.

@xcaruso @DavidAyotte @ymusleh

### 📝 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.
- [X] 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.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38303
Reported by: Antoine Leudière
Reviewer(s): Travis Scrimshaw, Xavier Caruso
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants