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

Support base_morphism for hom(im_gens) #26105

Closed
saraedum opened this issue Aug 21, 2018 · 54 comments
Closed

Support base_morphism for hom(im_gens) #26105

saraedum opened this issue Aug 21, 2018 · 54 comments

Comments

@saraedum
Copy link
Member

Currently, the method .hom(im_gens) on parents allows you to define the images of the generators of a structure. This is assuming that the coefficients of these generators can be sent to the codomain with some canonical coercion.

This is often insufficient, e.g., in the case of polynomial rings where you would also like to define a map that translate the ring of coefficients non-canonically. Note that function fields already have a specialized .hom() implementation that supports this feature.

See #26103 for a followup.

CC: @roed314 @xcaruso @sagetrac-swewers

Component: coercion

Keywords: padicBordeaux

Author: Julian Rüth, David Roe

Branch/Commit: 626f4ca

Reviewer: David Roe, Julian Rüth, Xavier Caruso

Issue created by migration from https://trac.sagemath.org/ticket/26105

@saraedum saraedum added this to the sage-8.4 milestone Aug 21, 2018
@saraedum
Copy link
Member Author

comment:1

roed, caruso: I am curious to hear what you think about this. I have been stumbling upon this several times during the past years.

@saraedum

This comment has been minimized.

@jdemeyer
Copy link

comment:3

The problem with im_gens is that it's not really mathematically defined in the first place. So I wouldn't try to add more functionality to it.

Instead, I would define a specific morphism class for polynomial rings. Mathematically, a ring morphism from a polynomial ring is defined by a morphism of the base together with the images of the generators. That was my plan for #25558 but I stopped working on that...

@saraedum
Copy link
Member Author

comment:4

Polynomial rings are just one example. Another one (see #26103) are extensions, say an extension of a finite field where I would like to define a map k(α)→l.

But I think I also had this issue with fraction fields before.

I agree, that specialized morphism classes can be nice because they might have more knowledge about the objects involved (say to implement is_injective()/is_surjective()) but I don't want to implement a new class whenever I need a map between new types of parents and generic code might do that as well.

Maybe I'll just use a SetMorphism if I shouldn't extend hom() though I personally don't like these.

Anyway, let me give a try at extending hom() and see what the result looks like.

@saraedum
Copy link
Member Author

comment:5

Looking at the ring homomorphism code brings back bad memories from #23204. Maybe I don't want to add more to morphism.pyx and homset.py as these should probably be drastically simplified if anything.

@jdemeyer
Copy link

comment:6

Replying to @saraedum:

I agree, that specialized morphism classes can be nice because they might have more knowledge about the objects involved (say to implement is_injective()/is_surjective()) but I don't want to implement a new class whenever I need a map between new types of parents and generic code might do that as well.

Of course, if it works using generic code, that's fine for me. But it might be harder to do it that way. I'm pretty sure that some of the existing issues with morphism are precisely because code wants to be too generic and doesn't really know what it is doing.

@jdemeyer
Copy link

comment:7

Replying to @saraedum:

Another one (see #26103) are extensions, say an extension of a finite field where I would like to define a map k(α)→l.

But this is really a map induced by a polynomial ring map k[x] → l. We already have a class for such maps induced on a quotient, so we just need the polynomial ring map.

@saraedum
Copy link
Member Author

comment:8

Replying to @jdemeyer:

Replying to @saraedum:

Another one (see #26103) are extensions, say an extension of a finite field where I would like to define a map k(α)→l.

But this is really a map induced by a polynomial ring map k[x] → l. We already have a class for such maps induced on a quotient, so we just need the polynomial ring map.

I am not sure I understand. If k is not a prime field, then how would I would I define my map k[x]→l?

@jdemeyer
Copy link

comment:9

Replying to @saraedum:

I am not sure I understand. If k is not a prime field, then how would I would I define my map k[x]→l?

By giving a map k → l and the image of x.

If k is not a prime finite field, then use recursion: k = f[α], so k → l is induced by a map f[x] → l.

@saraedum
Copy link
Member Author

comment:10

Sorry, my question was how to do this in Sage precisely. So, I can map k(α)→k[x] by mapping the generator to x. Then I map k[x]→l[x] by using an induced map. Finally I use the evaluation l[x]→l. But are people really supposed to figure this out on their own?

If I understand your proposal in comment 7 above, you would like to see a shortcut to define k[x]→l as (k[x]).hom(α', base_morphism=k→l). But why not go all the way and allow people to write (k(α)).hom(α', base_morphism=k→l)?

@jdemeyer
Copy link

comment:11

Replying to @saraedum:

But why not go all the way and allow people to write (k(α)).hom(α', base_morphism=k→l)?

I'm only talking about the implementation. What you "allow people to write" is an entirely different discussion.

@saraedum
Copy link
Member Author

comment:12

An alternative might be to just document these things in the docstring of hom().

@xcaruso
Copy link
Contributor

xcaruso commented Aug 29, 2018

comment:13

@saraedum:
I'm not sure that I understand what you have in mind. Could you please give a short concrete example of what you would like to see implemented, please?

@saraedum
Copy link
Member Author

comment:14

A concrete example where I would like to be able to specify a base_morphism is the following:

sage: k = GF(2)
sage: R.<a> = k[]
sage: l.<a> = k.extension(a^3 + a^2 + 1)
sage: R.<b> = l[]
sage: m.<b> = l.extension(b^2 + b + a)
sage: n.<z> = GF(2^6)

sage: m.hom([z^4 + z^3 + 1], base_morphism=l.hom([z^5 + z^4 + z^2]))

@saraedum
Copy link
Member Author

Changed keywords from none to padicBordeaux

@roed314
Copy link
Contributor

roed314 commented Sep 11, 2019

Branch: u/roed/26105_base_hom

@roed314
Copy link
Contributor

roed314 commented Sep 11, 2019

comment:17

I fixed a few other things as well:

  • a whitespace issue in parent.pyx (a function indented three spaces rather than 4)
  • Changed some double underscore attributes to single underscore
  • Switched some custom caching to cached_method.

New commits:

d4a35acAdd base_map to homomorphisms defined by images of generators
1f78dd4Fix some doctests

@roed314
Copy link
Contributor

roed314 commented Sep 11, 2019

Commit: 1f78dd4

@saraedum
Copy link
Member Author

Reviewer: David Roe, Julian Rüth

@saraedum
Copy link
Member Author

Changed author from Julian Rüth to Julian Rüth, David Roe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 12, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

84704baFix doctests and py3, incorporate small reviewer suggestion

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 12, 2019

Changed commit from 1f78dd4 to 84704ba

@xcaruso
Copy link
Contributor

xcaruso commented Sep 12, 2019

comment:20

Replying to @saraedum:

A concrete example where I would like to be able to specify a base_morphism is the following:

sage: k = GF(2)
sage: R.<a> = k[]
sage: l.<a> = k.extension(a^3 + a^2 + 1)
sage: R.<b> = l[]
sage: m.<b> = l.extension(b^2 + b + a)
sage: n.<z> = GF(2^6)

sage: m.hom([z^4 + z^3 + 1], base_morphism=l.hom([z^5 + z^4 + z^2]))

This example raises the following error with the current implementation:

Traceback (most recent call last)
...
ValueError: relations do not all (canonically) map to 0 under map determined by images of generators

@xcaruso
Copy link
Contributor

xcaruso commented Sep 12, 2019

Changed author from Julian Rüth, David Roe to Julian Rüth, David Roe, Xavier Caruso

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

3d73776Fixing Lie algebra morphisms

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2019

Changed commit from 3d73776 to 72b677c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

72b677cchange_ring -> map_coefficients and fix composition of morphisms defined by images of generators

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2019

Changed commit from 72b677c to d780c6f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

d780c6fChange base_map so that the codomain is always the codomain of the map

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

bfcbebeFix bug in composition

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2019

Changed commit from d780c6f to bfcbebe

@xcaruso
Copy link
Contributor

xcaruso commented Oct 3, 2019

comment:37

LGTM (see also discussion on Zulip).

Positive review when patchbot is happy.

@xcaruso
Copy link
Contributor

xcaruso commented Oct 4, 2019

comment:38

There are failing doctests.

File "src/sage/rings/morphism.pyx", line 837, in sage.rings.morphism.RingHomomorphism._composition_
Failed example:
    (f*f).base_map()
Expected:
    Ring morphism:
      From: Univariate Polynomial Ring in x over Rational Field
      To:   Univariate Polynomial Ring in y over Univariate Polynomial Ring in x over Rational Field
      Defn: x |--> 2*x
Got:
    Ring morphism:
      From: Univariate Polynomial Ring in x over Rational Field
      To:   Univariate Polynomial Ring in y over Univariate Polynomial Ring in x over Rational Field
      Defn: x |--> 2*x
            with map of base ring
**********************************************************************
File "src/sage/rings/morphism.pyx", line 1117, in sage.rings.morphism.RingHomomorphism_im_gens.__init__
Failed example:
    phi = S.hom([xx+1,xx-1],check=False)
Expected:
    Traceback (most recent call last):
    ...
    TypeError: images do not define a valid homomorphism
Got:
    <BLANKLINE>
**********************************************************************
File "src/sage/rings/morphism.pyx", line 1192, in sage.rings.morphism.RingHomomorphism_im_gens.base_map
Failed example:
    phi.base_map()
Expected:
    Ring endomorphism of Number Field in i with defining polynomial x^2 + 1
      Defn: i |--> -i
Got:
    Composite map:
      From: Number Field in i with defining polynomial x^2 + 1
      To:   Univariate Polynomial Ring in y over Number Field in i with defining polynomial x^2 + 1
      Defn:   Ring endomorphism of Number Field in i with defining polynomial x^2 + 1
              Defn: i |--> -i
            then
              Polynomial base injection morphism:
              From: Number Field in i with defining polynomial x^2 + 1
              To:   Univariate Polynomial Ring in y over Number Field in i with defining polynomial x^2 + 1
**********************************************************************
File "src/sage/structure/parent_gens.pyx", line 311, in sage.structure.parent_gens.ParentWithGens.gens.hom
Failed example:
    f = R.hom([x+1], base_map=lambda t: t+1); f
Exception raised:
    Traceback (most recent call last):
      File "/local/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/local/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1123, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.structure.parent_gens.ParentWithGens.gens.hom[22]>", line 1, in <module>
        f = R.hom([x+Integer(1)], base_map=lambda t: t+Integer(1)); f
      File "sage/structure/parent_gens.pyx", line 328, in sage.structure.parent_gens.ParentWithGens.hom (build/cythonized/sage/structure/parent_gens.c:3690)
        return parent.Parent.hom(self, im_gens, codomain, base_map=base_map, category=category, check=check)
      File "sage/structure/parent.pyx", line 1360, in sage.structure.parent.Parent.hom (build/cythonized/sage/structure/parent.c:11708)
        return self.Hom(codomain, **Hom_kwds)(im_gens, **kwds)
      File "/local/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/rings/homset.py", line 208, in __call__
        return morphism.RingHomomorphism_im_gens(self, im_gens, base_map=base_map, check=check)
      File "sage/rings/morphism.pyx", line 1134, in sage.rings.morphism.RingHomomorphism_im_gens.__init__ (build/cythonized/sage/rings/morphism.c:8239)
        if base_map.codomain() is not self.codomain():
    AttributeError: 'function' object has no attribute 'codomain'
**********************************************************************
File "src/sage/structure/parent_gens.pyx", line 315, in sage.structure.parent_gens.ParentWithGens.gens.hom
Failed example:
    f.category_for()
Expected:
    Join of Category of euclidean domains and Category of commutative algebras over (finite enumerated fields and subquotients of monoids and quotients of semigroups) and Category of infinite sets
Got:
    Category of enumerated euclidean domains
**********************************************************************
File "src/sage/structure/parent_gens.pyx", line 317, in sage.structure.parent_gens.ParentWithGens.gens.hom
Failed example:
    f(-1)
Expected:
    0
Got:
    4
**********************************************************************

And the flag is red for non_ascii and pyflakes but I don't understand what's wrong.

@xcaruso
Copy link
Contributor

xcaruso commented Oct 6, 2019

Changed branch from u/roed/26105_base_hom to u/caruso/26105_base_hom

@xcaruso
Copy link
Contributor

xcaruso commented Oct 6, 2019

Changed commit from bfcbebe to 626f4ca

@xcaruso
Copy link
Contributor

xcaruso commented Oct 6, 2019

comment:40

I've just updated the doctest.

If you agree with my changes, please give a positive review.


New commits:

a62a47cMerge branch 'u/roed/26105_base_hom' of git://trac.sagemath.org/sage into base_hom
5324a83Merge branch 'develop' into base_hom
2f526f8Merge branch 'u/roed/26105_base_hom' of git://trac.sagemath.org/sage into base_hom
3705163Merge branch 'u/roed/26105_base_hom' of git://trac.sagemath.org/sage into base_hom
243e72aMerge branch 'develop' into base_hom
626f4cafix doctest

@roed314
Copy link
Contributor

roed314 commented Oct 10, 2019

comment:41

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Oct 12, 2019

Changed branch from u/caruso/26105_base_hom to 626f4ca

@vbraun vbraun closed this as completed in f8f5ac2 Oct 12, 2019
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 23, 2023
, sagemath#24483, sagemath#24371, sagemath#24511, sagemath#25848, sagemath#26105, sagemath#28481, sagemath#29010, sagemath#29412, sagemath#30332, sagemath#30372, sagemath#31345, sagemath#32375, sagemath#32606, sagemath#32610, sagemath#32612, sagemath#32641, sagemath#32660, sagemath#32750, sagemath#32869, sagemath#33602

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36307
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants