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:

### 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 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 4 years ago by vdelecroix (previous) (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.