Opened 3 years ago
Closed 3 years ago
#21106 closed enhancement (fixed)
class for flattening polynomial rings over polynomial rings
Reported by:  bhutz  Owned by:  

Priority:  minor  Milestone:  sage7.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 3 years ago by
 Branch set to u/bhutz/flatten
comment:2 Changed 3 years ago by
 Commit set to cf97fed2b1dba25909f952e899e2256a87b092d5
comment:3 Changed 3 years ago by
 Cc vdelecroix added
comment:4 Changed 3 years ago by
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 3 years ago by
The error ValueError("clash in variable names")
should be tested.
comment:6 Changed 3 years ago by
I am not sure we want that FlatteningMorphism
in the global namespace.
comment:7 Changed 3 years ago by
Those are all simple enough. I also removed some terminating white space
comment:8 Changed 3 years ago by
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 3 years ago by
 Commit changed from cf97fed2b1dba25909f952e899e2256a87b092d5 to 957589f9db22ccd6acddb1f6c5bb4c217b7be1d8
Branch pushed to git repo; I updated commit sha1. New commits:
957589f  21106: minor fixes

comment:10 Changed 3 years ago by
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
comment:11 Changed 3 years ago by
Do you think we want to support both version of _call_
?
comment:12 Changed 3 years ago by
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 3 years ago by
Note that a similar _call_
can be implemented for the section morphism.
comment:14 Changed 3 years ago by
 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 3 years ago by
 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 3 years ago by
Is it faster than string?
comment:17 followup: ↓ 18 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
Strings are also bad for floating point
sage: a = RR(1 / 2**30) sage: RR(str(a)) == a False
comment:20 Changed 3 years ago by
 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 3 years ago by
yes, those are good and caught an error in the codomain as well that I've corrected.
comment:22 Changed 3 years ago by
 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 3 years ago by
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 3 years ago by
 Status changed from needs_review to positive_review
looks good to me. Thanks.
comment:25 Changed 3 years ago by
 Status changed from positive_review to needs_work
comment:26 Changed 3 years ago by
problem with the file mode... it is rwxrxrx
instead of rwrr
.
(incidentally I have another commit to propose)
comment:27 Changed 3 years ago by
 Commit changed from f05f7db9f9e31697cb2be98b2cf31533a44e09e2 to 19cb1719e166126f6ffd8d8a9b33c2262b252851
comment:28 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:29 Changed 3 years ago by
 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 3 years ago by
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 3 years ago by
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 3 years ago by
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/sagedevel/CDLCb6OX4ns
comment:33 Changed 3 years ago by
 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 3 years ago by
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.
comment:35 Changed 3 years ago by
ok, so I should mark this back to positive, and then open a new ticket to fix the issue?
comment:36 Changed 3 years ago by
better, yes.
comment:37 Changed 3 years ago by
 Status changed from needs_work to positive_review
the issue is now #21113
comment:38 Changed 3 years ago by
 Milestone changed from sage7.3 to sage7.4
comment:39 Changed 3 years ago by
 Branch changed from public/21106 to 19cb1719e166126f6ffd8d8a9b33c2262b252851
 Resolution set to fixed
 Status changed from positive_review to closed
Here is a first attempt to flesh this out. Please comment.
New commits:
21106: create polynomial ring flattening class