#21106 closed enhancement (fixed)

class for flattening polynomial rings over polynomial rings

Reported by: bhutz Owned by:
Priority: minor Milestone: sage-7.4
Component: algebra Keywords:
Cc: vdelecroix Merged in:
Authors: Ben Hutz, Vincent Delecroix Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 19cb171 (Commits) Commit: 19cb1719e166126f6ffd8d8a9b33c2262b252851
Dependencies: Stopgaps:

Description

Given a polynomial ring QQ['a',b']['x','y'] construct a morphism and its inverse to the ring QQ['a','b','x','y']

Change History (39)

comment:1 Changed 14 months ago by bhutz

  • Branch set to u/bhutz/flatten

comment:2 Changed 14 months ago by bhutz

  • Commit set to cf97fed2b1dba25909f952e899e2256a87b092d5

Here is a first attempt to flesh this out. Please comment.


New commits:

cf97fed21106: create polynomial ring flattening class

comment:3 Changed 14 months ago by vdelecroix

  • Cc vdelecroix added

comment:4 Changed 14 months ago by vdelecroix

For example ``QQ['a','b'],['x','y']`` flattens to ``QQ['a','b','c','d']``.

should I read ``QQ['a', 'b', 'x', 'y']``?

comment:5 Changed 14 months ago by vdelecroix

The error ValueError("clash in variable names") should be tested.

comment:6 Changed 14 months ago by vdelecroix

I am not sure we want that FlatteningMorphism in the global namespace.

comment:7 Changed 14 months ago by bhutz

Those are all simple enough. I also removed some terminating white space

comment:8 Changed 14 months ago by vdelecroix

Here is a recursive version of _call_ that seems to be a little bit faster

    def _call2_(self, p):
        r"""
        TESTS::

            sage: R = QQ['x']['y']['s','t']
            sage: p = R('s*x + y*t + x^2*s + 1 + t')
            sage: f = FlatteningMorphism(R)
            sage: f._call2_(p)
            x^2*s + x*s + y*t + t + 1
        """
        p = {(): p}

        for ring in self._intermediate_rings:
            new_p = {}
            if is_PolynomialRing(ring):
                for mon,pp in p.iteritems():
                    assert pp.parent() == ring
                    for i,j in pp.dict().iteritems():
                        new_p[(i,)+(mon)] = j
            elif is_MPolynomialRing(ring):
                for mon,pp in p.iteritems():
                    assert pp.parent() == ring
                    for mmon,q in pp.dict().iteritems():
                        new_p[tuple(mmon)+mon] = q
            else:
                raise RuntimeError
            p = new_p

    return self.codomain()(p)

comment:9 Changed 14 months ago by git

  • Commit changed from cf97fed2b1dba25909f952e899e2256a87b092d5 to 957589f9db22ccd6acddb1f6c5bb4c217b7be1d8

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

957589f21106: minor fixes

comment:10 Changed 14 months ago by vdelecroix

To use _call2_, the constructor needs to be modified with

        intermediate_rings = []
        ... # AS BEFORE
        while is_PolynomialRing(ring) or is_MPolynomialRing(ring):
            intermediate_rings.append(ring)
            ... # AS BEFORE
        ... # AS BEFORE
        self._intermediate_rings = intermediate_rings
Last edited 14 months ago by vdelecroix (previous) (diff)

comment:11 Changed 14 months ago by vdelecroix

Do you think we want to support both version of _call_?

comment:12 Changed 14 months ago by bhutz

None of the examples that I've tried fail for _call2_? The more constructive call2 sits better with me as it is not relying on iterpreting a string as a polynomial.

comment:13 Changed 14 months ago by vdelecroix

Note that a similar _call_ can be implemented for the section morphism.

comment:14 Changed 14 months ago by git

  • Commit changed from 957589f9db22ccd6acddb1f6c5bb4c217b7be1d8 to 8c22f750a6903a284a27a19f871fd93e5a875343

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

8c22f7521106: new version of _call_

comment:15 Changed 14 months ago by bhutz

  • Status changed from new to needs_review

changes made and a version of call2 for unflattening. Not quite as elegant as yours, but constructive.

comment:16 Changed 14 months ago by vdelecroix

Is it faster than string?

comment:17 follow-up: Changed 14 months ago by bhutz

much faster for things like CC and str does not work for QQbar. It is slightly slower for QQ.

comment:18 in reply to: ↑ 17 Changed 14 months ago by vdelecroix

Replying to bhutz:

much faster for things like CC and str does not work for QQbar. It is slightly slower for QQ.

Would be good to add doctest for QQbar. By the way, the string method would also fail for something like the following (that would be good to doctest as well)

sage: K.<a> = NumberField(x^3 - 2)
sage: R = K['x','y']['a','b']

comment:19 Changed 14 months ago by vdelecroix

Strings are also bad for floating point

sage: a = RR(1 / 2**30)
sage: RR(str(a)) == a
False

comment:20 Changed 14 months ago by git

  • Commit changed from 8c22f750a6903a284a27a19f871fd93e5a875343 to 570d9ee1380519d3c34d92c6ec329d9f1b6b36e4

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

570d9ee21106: added doc tests

comment:21 Changed 14 months ago by bhutz

yes, those are good and caught an error in the codomain as well that I've corrected.

comment:22 Changed 14 months ago by vdelecroix

  • Branch changed from u/bhutz/flatten to public/21106
  • Commit changed from 570d9ee1380519d3c34d92c6ec329d9f1b6b36e4 to f05f7db9f9e31697cb2be98b2cf31533a44e09e2
  • Reviewers set to Vincent Delecroix

New commits:

f05f7db21106:cleaning

comment:23 Changed 14 months ago by vdelecroix

In my commit

  • simplified your _call_ (and it is now faster)
  • removed some trailing whitespaces
  • add more intensive random doctests

Please review my changes. And if you are happy with them, you can set to positive review.

comment:24 Changed 14 months ago by bhutz

  • Status changed from needs_review to positive_review

looks good to me. Thanks.

comment:25 Changed 14 months ago by vdelecroix

  • Status changed from positive_review to needs_work

comment:26 Changed 14 months ago by vdelecroix

problem with the file mode... it is -rwxr-xr-x instead of -rw-r--r--.

(incidentally I have another commit to propose)

comment:27 Changed 14 months ago by git

  • Commit changed from f05f7db9f9e31697cb2be98b2cf31533a44e09e2 to 19cb1719e166126f6ffd8d8a9b33c2262b252851

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

6e5094221106: change file mode
19cb17121106: Python3 compatibility + more cleaning

comment:28 Changed 14 months ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:29 Changed 14 months ago by bhutz

  • Status changed from needs_review to positive_review

those changes are fine with me. I pulled it down to test just to be safe, and all still runs fine.

comment:30 Changed 14 months ago by vdelecroix

Thanks!

I also think that in the future the FlatteningMorphism should support QQ['a','b']['a','b']['a']['b']. There is almost nothing to modify to make the target to be QQ['x0','x1','x2','x3','x4','x5'] (I guess only the constructor).

comment:31 Changed 14 months ago by bhutz

You probably saw, I removed the original codomain argument. I was running into some difficulties there with _call_, but with the new version of _call_ it would probably actually be easy now.

comment:32 Changed 14 months ago by vdelecroix

Yep. It might also be that for some operation it is good to try in QQ['a']['b']['c']['d']['e'] instead. In which case we might want to use UnflattenMorphism without using FlattenMorphism. See my recent post

https://groups.google.com/forum/#!topic/sage-devel/CDLCb6OX4ns

comment:33 Changed 14 months ago by bhutz

  • Status changed from positive_review to needs_work

unflatteningmorphism cannot be used by itself since _intermediate rings is defined through section. I'll fix that shortly.

comment:34 Changed 14 months ago by vdelecroix

You are free to open a new ticket for that issue. Do not change a ticket in positive_review to needs_work. Moreover when it is in that state since a long time and that there are other tickets based on it.

Last edited 14 months ago by vdelecroix (previous) (diff)

comment:35 Changed 14 months ago by bhutz

ok, so I should mark this back to positive, and then open a new ticket to fix the issue?

comment:36 Changed 14 months ago by vdelecroix

better, yes.

comment:37 Changed 14 months ago by bhutz

  • Status changed from needs_work to positive_review

the issue is now #21113

comment:38 Changed 14 months ago by vbraun

  • Milestone changed from sage-7.3 to sage-7.4

comment:39 Changed 14 months ago by vbraun

  • Branch changed from public/21106 to 19cb1719e166126f6ffd8d8a9b33c2262b252851
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.