Opened 4 years ago

Closed 4 years ago

# class for flattening polynomial rings over polynomial rings

Reported by: Owned by: bhutz minor sage-7.4 algebra vdelecroix Ben Hutz, Vincent Delecroix Vincent Delecroix N/A 19cb171 (Commits) 19cb1719e166126f6ffd8d8a9b33c2262b252851

### Description

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

### comment:1 Changed 4 years ago by bhutz

• Branch set to u/bhutz/flatten

### comment:2 Changed 4 years ago by bhutz

• Commit set to cf97fed2b1dba25909f952e899e2256a87b092d5

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

New commits:

 ​cf97fed `21106: create polynomial ring flattening class`

### comment:4 Changed 4 years 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 4 years ago by vdelecroix

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

### comment:6 Changed 4 years ago by vdelecroix

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

### comment:7 Changed 4 years ago by bhutz

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

### comment:8 Changed 4 years 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 4 years ago by git

• Commit changed from cf97fed2b1dba25909f952e899e2256a87b092d5 to 957589f9db22ccd6acddb1f6c5bb4c217b7be1d8

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

 ​957589f `21106: minor fixes`

### comment:10 Changed 4 years 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 4 years ago by vdelecroix (previous) (diff)

### comment:11 Changed 4 years ago by vdelecroix

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

### comment:12 Changed 4 years 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 4 years ago by vdelecroix

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

### comment:14 Changed 4 years ago by git

• Commit changed from 957589f9db22ccd6acddb1f6c5bb4c217b7be1d8 to 8c22f750a6903a284a27a19f871fd93e5a875343

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

 ​8c22f75 `21106: new version of _call_`

### comment:15 Changed 4 years 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 4 years ago by vdelecroix

Is it faster than string?

### comment:17 follow-up: ↓ 18 Changed 4 years 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 4 years ago by vdelecroix

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 4 years 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 4 years ago by git

• Commit changed from 8c22f750a6903a284a27a19f871fd93e5a875343 to 570d9ee1380519d3c34d92c6ec329d9f1b6b36e4

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

 ​570d9ee `21106: added doc tests`

### comment:21 Changed 4 years ago by bhutz

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

### comment:22 Changed 4 years 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:

 ​f05f7db `21106:cleaning`

### comment:23 Changed 4 years 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 4 years ago by bhutz

• Status changed from needs_review to positive_review

looks good to me. Thanks.

### comment:25 Changed 4 years ago by vdelecroix

• Status changed from positive_review to needs_work

### comment:26 Changed 4 years 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 4 years ago by git

• Commit changed from f05f7db9f9e31697cb2be98b2cf31533a44e09e2 to 19cb1719e166126f6ffd8d8a9b33c2262b252851

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

 ​6e50942 `21106: change file mode` ​19cb171 `21106: Python3 compatibility + more cleaning`

### comment:28 Changed 4 years ago by vdelecroix

• Status changed from needs_work to needs_review

### comment:29 Changed 4 years 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 4 years 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 4 years 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 4 years 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

### comment:33 Changed 4 years 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 4 years ago by vdelecroix

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

Version 1, edited 4 years ago by vdelecroix (previous) (next) (diff)

### comment:35 Changed 4 years ago by bhutz

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

better, yes.

### comment:37 Changed 4 years ago by bhutz

• Status changed from needs_work to positive_review

the issue is now #21113

### comment:38 Changed 4 years ago by vbraun

• Milestone changed from sage-7.3 to sage-7.4

### comment:39 Changed 4 years 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.