sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.19k stars 412 forks source link

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

Open kryzar opened 1 week ago

kryzar commented 1 week ago

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

xcaruso commented 1 week ago

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 commented 1 week ago

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

DavidAyotte commented 1 week ago

I agree with what Xavier said!

kryzar commented 1 week ago

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