Opened 6 years ago
Closed 7 months ago
#18036 closed defect (fixed)
I.parent() should not be the symbolic ring
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  number fields  Keywords:  
Cc:  wuthrich, jdemeyer, mmezzarobba, behackl, rws, ghkliem, ghmwageringel  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  54a34a7 (Commits, GitHub, GitLab)  Commit:  54a34a7443d373e678c5461e5205ef2cdd7470b1 
Dependencies:  Stopgaps: 
Description (last modified by )
As suggested in #7545, this ticket defines the imaginary unit I
directly as the generator of QuadraticField(1)
instead of wrapping it in a symbolic expression.
Why? To allow it to be used in combination with elements of QQbar, CC, etc., without coercion forcing the expression to SR. For example, 1.0 + I
is now an element of CC instead of SR.
How? We set I
in sage.all to the generator of ℚ[i], and deprecate importing it from sage.symbolic.I
. The symbolic I
remains available from sage.symbolic.constants
for library code working with symbolic expressions, and as SR(I)
or SR.I()
. We create a dedicated subclass of quadratic number field elements to make it possible to support features similar to those of symbolic expressions of the form a + I*b
that would not make sense for number field elements (or be too hard to implement, or pollute the namespace).
Why not ℤ[i]? Because the class hierarchy of number field and order elements makes it difficult to provide the compatibility features mentioned above for elements of both ℤ[i] and ℚ[i]. Having I
be an element of ℚ[i] covers almost all use cases (all except working with algebraic integers?), and people who work with orders are sophisticated enough to explicitly ask for I ∈ ℤ[i] when they need that. (This is a debatable choice. We could probably do without the dedicated subclass for elements of ℚ[i], at the price of breaking backward compatibility a bit more.)
Change History (78)
comment:1 followups: ↓ 2 ↓ 4 Changed 6 years ago by
comment:2 in reply to: ↑ 1 ; followup: ↓ 5 Changed 6 years ago by
Replying to nbruin:
I'm not so sure it should. Which quadratic field is the appropriate one? There are many, distinguishable by the name of their generator (that would be 'I') for this one, but also by their specified embeddings, and it's not clear which one to choose.
I also thought of this in the train... and I do not see much possible choices. I found two rather natural choices for the adoption of I
:
 the ring of integers
Z[sqrt(1)]
with its natural embedding inQQbar
QQbar
itself
Is there an argument for doing this? Ticket #17860 referenced in the description makes no mention of it. I'd imagine there might be evaluation reasons that might make it attractive. Perhaps those also give an indication of which quadratic field would be the appropriate one.
Some reasons (in favor of the first choice):
I + 1.0
and1.0 * I
should be complex numbersfactor((I+3))
should be the factorization over the Gaussian integers (i.e.(I) * (I + 1) * (2*I + 1)
)abs(I)
should be the integer1
Vincent
comment:3 Changed 6 years ago by
I
should be an element of QuadraticField(1, 'I', embedding=CC.gen(), latex_name='i')
, which is what it currently is (see src/sage/symbolic/pynac.pyx
).
comment:4 in reply to: ↑ 1 Changed 6 years ago by
Replying to nbruin:
Is there an argument for doing this?
In short: the same reason that 1
is not symbolic. When doing basic arithmetic with I
, there is no need for a symbolic I
. Whenever something symbolic is needed, coercion will make it symbolic.
comment:5 in reply to: ↑ 2 Changed 6 years ago by
Replying to vdelecroix:
I found two rather natural choices for the adoption of
I
:
 the ring of integers
Z[sqrt(1)]
with its natural embedding inQQbar
The ring ZZ[sqrt(1)]
definitely looks like the most natural choice to me, since admits a canonical homomorphism to any other ring with a distinguished square root of 1.
As for the distinguished embedding, is there a specific reason for choosing QQbar
? A more minimal choice would be to fix an embedding into a UniversalCyclotomicField
; then we would have coercion maps ZZ[I]
> UniversalCyclotomicField(zeta)
> QQbar
> CC
. (Maybe this makes finding common parents slightly harder, though.)
comment:6 Changed 6 years ago by
 Description modified (diff)
Thank you all for working on this  this kind of thing has been on the radar for years but after Burcin left daytoday operations around here there hasn't been the combination of energy and knowhow to do this "correctly", whatever that might mean. Just keep in mind it would be nice for I in SR
to be true, though I'm sure it will be since 1 in SR
already is True
. I do like the idea of abs(I)
being an Integer
and not a symbolic expression.
comment:7 Changed 6 years ago by
 Cc mmezzarobba added
comment:8 Changed 6 years ago by
Proof of concept to see what would break: u/mmezzarobba/18036QQi
(I'm using a number field element for now, not an order element, but switching shouldn't be hard). Still needs quite a bit of work, but all tests should pass (I didn't run them all with the last version of the code). Any comment or improvement welcome!
In particular:
 Are there behavior changes that you think are not acceptable, or not acceptable without a deprecation?
 I'm not happy with the changes to
sage.geometry.hyperbolic_space
(which apparently relied on operations involvingI
triggering coercions toSR
), but I don't understand the code well enough to do better.
Tangentially related: now may be a good time to deprecate (or remove directly?) the bogus coercion from SR
to QQbar
.
comment:9 Changed 6 years ago by
comment:10 followup: ↓ 11 Changed 6 years ago by
Very nice that it worked!
I do not quite understand why you need the creation of a new class of NumberFieldQQi
... is that only for the special method you need in the element class?
For embedding in QQbar I guess that what should be fixed is embedding of number fields. In the ideal world, you would declare:
QQi = NumberField(x**2 + 1, 'I', embedding=QQbar.gen())
But then, there might be an infinite loop with the definition of I
in QQbar
. I had the same sort of troubles when refining default embedding from lazy field to AA/QQbar
.
Vincent
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 6 years ago by
Replying to vdelecroix:
I do not quite understand why you need the creation of a new class of
NumberFieldQQi
... is that only for the special method you need in the element class?
I don't remember, it could be that the reason no longer exists due to later changes.
For embedding in QQbar I guess that what should be fixed is embedding of number fields. In the ideal world, you would declare:
QQi = NumberField(x**2 + 1, 'I', embedding=QQbar.gen())
Yes, the idea is to switch to an embedding into QQbar when other number fields do.
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 6 years ago by
Replying to mmezzarobba:
Replying to vdelecroix:
I do not quite understand why you need the creation of a new class of
NumberFieldQQi
... is that only for the special method you need in the element class?I don't remember, it could be that the reason no longer exists due to later changes.
One reason was that having separate classes makes it easy to test if we are in the special case of QQ[i] using isinstance
. In the case of the parent class, this is convenient when specifying coercions, for instance.
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 6 years ago by
Replying to mmezzarobba:
Replying to mmezzarobba:
Replying to vdelecroix:
I do not quite understand why you need the creation of a new class of
NumberFieldQQi
... is that only for the special method you need in the element class?I don't remember, it could be that the reason no longer exists due to later changes.
One reason was that having separate classes makes it easy to test if we are in the special case of QQ[i] using
isinstance
. In the case of the parent class, this is convenient when specifying coercions, for instance.
Anyway this will be instantiated at startup so why not keeping one instance QQi
in sage.rings.number_field.number_field
? (like we have for ZZ
, QQ
, etc). Then you can test identity when testing coercions.
comment:14 in reply to: ↑ 13 ; followup: ↓ 16 Changed 6 years ago by
Replying to vdelecroix:
Anyway this will be instantiated at startup so why not keeping one instance
QQi
insage.rings.number_field.number_field
? (like we have forZZ
,
If I remember right, currently, just adding QQi = ...()
in the module currently doesn't work due to import order constraints. For now I just kept the creation of QQ[i] happening at the same time as it used to. But that's certainly something we should try to improve after this first draft.
Then you can test identity when testing coercions.
Yes. Having a separate class would also be natural if we want I.parent()
to display something less frightening than “Number Field in I
with defining polynomial x^2 + 1
”, and more generally to implement features specific to QQ[i]. But I can't really think of anything that makes sense for this field and not for embedded quadratic number fields in general, so perhaps it is better to encourage people to always implement a more general version?
A related question is whether QQi is NumberField(x^2+1, 'I', embedding=CC.0)
should be true, or if there should be two separate parents.
What do you think?
comment:15 Changed 6 years ago by
 Summary changed from I should not be symbolic to I.parent() should not be the symbolic ring
comment:16 in reply to: ↑ 14 ; followup: ↓ 17 Changed 6 years ago by
Replying to mmezzarobba:
Replying to vdelecroix:
Anyway this will be instantiated at startup so why not keeping one instance
QQi
insage.rings.number_field.number_field
? (like we have forZZ
,If I remember right, currently, just adding
QQi = ...()
in the module currently doesn't work due to import order constraints. For now I just kept the creation of QQ[i] happening at the same time as it used to. But that's certainly something we should try to improve after this first draft.
Here we can probably cheat with
_QQi = None def NumberFieldQQi(): if _QQi is None: # build it once for all ... return _QQi
Then you can test identity when testing coercions.
Yes. Having a separate class would also be natural if we want
I.parent()
to display something less frightening than “Number Field inI
with defining polynomialx^2 + 1
”, and more generally to implement features specific to QQ[i]. But I can't really think of anything that makes sense for this field and not for embedded quadratic number fields in general, so perhaps it is better to encourage people to always implement a more general version?
Yes! Having a custom representation should be done in the main class. It is already possible:
sage: K = QuadraticField(2) sage: K.rename('It's me') sage: K It's me sage: K.rename(None) sage: K Number Field in a with defining polynomial x^2  2
A related question is whether
QQi is NumberField(x^2+1, 'I', embedding=CC.0)
should be true, or if there should be two separate parents.What do you think?
More generally, do we want unique representation for (absolute) number fields? I would tend to say yes. And the natural keys would be:
 the polynomial
 the variable name (not of the polynomial!)
 the embedding
Vincent
comment:17 in reply to: ↑ 16 ; followup: ↓ 18 Changed 6 years ago by
Replying to vdelecroix:
Yes! Having a custom representation should be done in the main class. It is already possible:
If all we want is a different string representation, yes, perhaps it makes sense to use rename()
...
A related question is whether
QQi is NumberField(x^2+1, 'I', embedding=CC.0)
should be true, or if there should be two separate parents.What do you think?
More generally, do we want unique representation for (absolute) number fields?
I think everyone agrees that absolute number fields should have unique representation. My question was whether Q[i] should be an absolute number field in this sense, or if it should be a “special” object such that people could work with both Q[i]asasubsetofcomplexnumbers and Q[i]asanumber field, possibly at the same time. I'd prefer a single object as well, but I am sure I have missed some of the implications, so if anyone has arguments in favor of the other option, I would be interested in hearing them.
comment:18 in reply to: ↑ 17 ; followup: ↓ 20 Changed 6 years ago by
Replying to mmezzarobba:
A related question is whether
QQi is NumberField(x^2+1, 'I', embedding=CC.0)
should be true, or if there should be two separate parents.What do you think?
More generally, do we want unique representation for (absolute) number fields?
I think everyone agrees that absolute number fields should have unique representation. My question was whether Q[i] should be an absolute number field in this sense, or if it should be a “special” object such that people could work with both Q[i]asasubsetofcomplexnumbers and Q[i]asanumber field, possibly at the same time. I'd prefer a single object as well, but I am sure I have missed some of the implications, so if anyone has arguments in favor of the other option, I would be interested in hearing them.
I would be interested in working with any number field seeing them as a subfield of the real or complex numbers! Not only QQi
and it makes sense to ask whether we need a dedicated class for that. For both parent and elements.
Note that it is already partly possible to play with element of number fields as real numbers (especially with quadratic fields)
sage: K.<sqrt2> = QuadraticField(2) sage: 1 < sqrt2 < 3/2 True sage: sqrt2.n() 1.41421356237310 sage: sqrt2 + CC(0,1) 1.41421356237310 + 1.00000000000000*I sage: sage: cos(sqrt2) cos(sqrt(2)) sage: sqrt2.continued_fraction() [1; (2)*]
About having methods .cos()
, .sin()
, .exp()
, it is already something which I found dangerous with integers for which the method .sqrt()
might return an answer with a different parent
sage: 4.sqrt() # answer is a Sage integer 2 sage: 2.sqrt() # answer is symbolic sqrt(2)
Which is very different from
sage: R.<x> = ZZ[] sage: ((x+1)**2 * (x2)**4).sqrt() x^3  3*x^2 + 4 sage: R(2).sqrt() Traceback (most recent call last): ... TypeError: Polynomial is not a square. You must specify the name of the square root when using the default extend = True
At the time Sage would support embedding of number fields into padic fields, I think it might be worse to have that dedicated class! But in the meantime, I have no strong opinion.
Vincent
comment:19 Changed 6 years ago by
comment:20 in reply to: ↑ 18 Changed 5 years ago by
Replying to vdelecroix:
About having methods
.cos()
,.sin()
,.exp()
, it is already something which I found dangerous with integers
Still, if we ever want this new nonsymbolic I
to behave like to old symbolic I
, we would need to support things like that:
********************************************************************** File "src/sage/symbolic/expression.pyx", line 7901, in sage.symbolic.expression.Expression.log Failed example: I.log() Exception raised: Traceback (most recent call last): File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 858, in compile_and_execute exec(compiled, globs) File "<doctest sage.symbolic.expression.Expression.log[11]>", line 1, in <module> I.log() File "sage/structure/element.pyx", line 420, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4676) return getattr_from_other_class(self, P._abstract_element_class, name) File "sage/structure/misc.pyx", line 259, in sage.structure.misc.getattr_from_other_class (build/cythonized/sage/structure/misc.c:1772) raise dummy_attribute_error AttributeError: 'sage.rings.number_field.number_field_element_quadratic.NumberFieldElement_quadratic' object has no attribute 'log' **********************************************************************
comment:21 Changed 5 years ago by
 Branch set to u/jdemeyer/i_parent___should_not_be_the_symbolic_ring
comment:22 Changed 5 years ago by
 Cc behackl added
 Commit set to 2f5a5198849c5b96bc2733e1e5853dc1b23f1cf9
New commits:
2f5a519  parent(I) should be a number field

comment:23 Changed 5 years ago by
 Cc rws added
comment:24 followups: ↓ 25 ↓ 26 Changed 5 years ago by
Hi! I'd like to revive this discussion a bit because we're getting a doctest failure at https://github.com/pynac/pynac/pull/162 which would probably be fixed along the lines of this ticket.
For starters, I had a look at the current branch and some of the resulting doctest failures; this should roughly resemble the tasks that are still to be done:
sage t warnlong 54.2 src/sage/symbolic/constants.py # 3 doctests failed
;
sage t warnlong 54.2 src/sage/symbolic/pynac.pyx # 3 doctests failed
:
duplicate doctests. not sure how to fix (maybe switch to I_symbolic?). and which to remove.
sage t warnlong 54.2 src/sage/symbolic/relation.py # 3 doctests failed
:
Doctests can be fixed directly.
sage t warnlong 54.2 src/sage/symbolic/expression_conversions.py # 2 doctests failed
probably I_symbolic is needed?
sage t warnlong 54.2 src/sage/symbolic/expression.pyx # 14 doctests failed
 arithmetic with
oo
is broken (4 doctests) 
_is_registered_constant_
> remove doctest? I_symbolic? 
is_numeric
/is_constant
missing (I_symbolic?) 
imag_part
/real_part
should be an alias ofimag
/real
(3 doctests) 
I.log
not implemented (3 doctests) rectform
: either converse to SR or multiply with I_symbolic.
 arithmetic with
sage t warnlong 54.2 src/sage/rings/infinity.py # 2 doctests failed
arithmetic with
oo
is broken.
sage t warnlong 54.2 src/sage/rings/complex_mpc.pyx # 8 doctests failed
andsage t warnlong 54.2 src/sage/rings/complex_arb.pyx # 7 doctests failed
 coercion errors:
ValueError: Cannot coerce algebraic number with nonzero imaginary part to algebraic real
 precision regressions.
 coercion errors:
sage t warnlong 54.2 src/sage/rings/qqbar.py # 6 doctests failed
common parent issue:
TypeError: unsupported operand parent(s) for '+': 'Algebraic Field' and 'Number Field in I with defining polynomial x^2 + 1'
sage t warnlong 54.2 src/sage/rings/number_field/number_field_element_quadratic.pyx # 1 doctest failed
common parent issue:
TypeError: unsupported operand parent(s) for '*': 'Number Field in I with defining polynomial x^2 + 1' and 'Number Field in sqrt3 with defining polynomial x^2  3'
sage t warnlong 54.2 src/sage/rings/polynomial/polynomial_rational_flint.pyx # 1 doctest failed
false error is raised
sage t warnlong 54.2 src/sage/rings/polynomial/cyclotomic.pyx # 2 doctests failed
both can be fixed directly.
 Stuff in
sage/geometry/hyperbolic_space/hyperbolic_*.py
breaks down:sage t warnlong 51.1 src/sage/geometry/hyperbolic_space/hyperbolic_point.py # 1 doctest failed
sage t warnlong 51.1 src/sage/geometry/hyperbolic_space/hyperbolic_geodesic.py # 4 doctests failed
sage t warnlong 51.1 src/sage/geometry/hyperbolic_space/hyperbolic_isometry.py # 1 doctest failed
sage t warnlong 51.1 src/sage/geometry/hyperbolic_space/hyperbolic_model.py # 2 doctests failed
Not sure about these errors, some seem to be coercion related, others are just
TypeError: 'sage.rings.complex_number.ComplexNumber' object is not callable
maybe that goes away along the way.
These are certainly not all failures, but they should give an idea of what breaks down. The biggest issue seems to be coercion...
comment:25 in reply to: ↑ 24 Changed 5 years ago by
comment:26 in reply to: ↑ 24 Changed 5 years ago by
Replying to behackl:
sage t warnlong 54.2 src/sage/rings/infinity.py # 2 doctests failed
arithmetic with
oo
is broken.
It's just missing coercion of number field elements into the infinity ring. It would give a SignError
anyway.
comment:27 followup: ↓ 28 Changed 5 years ago by
Would
A = GaussianIntegers() A(1+I)
work with this branch?
comment:28 in reply to: ↑ 27 Changed 5 years ago by
Replying to kcrisman:
sage: A = GaussianIntegers() sage: A(1+I) I + 1 sage: type(_) <type 'sage.rings.number_field.number_field_element_quadratic.OrderElement_quadratic'>
comment:29 Changed 5 years ago by
Thanks! That is definitely a motivation for me to review this next week ... any sense on whether the various doctest failures are "real", as opposed to just fixes like comment:26?
comment:30 Changed 5 years ago by
What's a "real" doctest failure? Some features of SR
simply are not supported for number field elements.
comment:31 followups: ↓ 32 ↓ 33 Changed 5 years ago by
So, we must have two different I
s.
This interface duplication creates a similar problems to the one with pos.char. elements (either ring elements or symbolic Mod
). I think what's most understandable to the user would be a switch that prevents mixing and whose value is clearly visible on startup like symbolic I mode is ON
. I know this goes against the no globals rules but I predict no one will accept having to input symbolic_I
, and we will get hell for that.
comment:32 in reply to: ↑ 31 Changed 5 years ago by
Replying to rws:
So, we must have two different
I
s.
I've lost track what the exact issues are that lead to that conclusion, but I would definitely prefer if we don't do that. I think we can avoid it for the following reason:
Sage itself needs *many* different I's. For one thing, every ComplexField(b)
needs its own. We'd like coercion to take care of creating the many different I's. I guess we're now finding it's difficult to find a parent that coerces into sufficiently many complex parents.
If that is the case, perhaps we should solve the problem by just having a LiteralI
, just as we have RealLiteral
to avoid the precision coercion problems that arise with floats. The advantage is that its coercion properties can be tailored exactly to what we need (within the bounds of what coercion can handle in the first place)
It doesn't surprise me that I is problematic that way. It is, after all, an object that represents multiple values.
comment:33 in reply to: ↑ 31 ; followup: ↓ 34 Changed 5 years ago by
Replying to rws:
So, we must have two different
I
s.
We really should not do that. Ideally, I
should be some number field element but which allows the appropriate symbolic operations.
comment:34 in reply to: ↑ 33 Changed 5 years ago by
Replying to jdemeyer:
Ideally,
I
should be some number field element but which allows the appropriate symbolic operations.
Well, if the operation's name is a registered function , convert to symbolic, apply, convert back?
comment:35 Changed 4 years ago by
See also #22208.
comment:36 Changed 4 years ago by
 Branch changed from u/jdemeyer/i_parent___should_not_be_the_symbolic_ring to public/18036
comment:37 Changed 3 years ago by
 Commit changed from 2f5a5198849c5b96bc2733e1e5853dc1b23f1cf9 to d7fab0b6a8628301543ed7bfae70380e1d6fa714
Branch pushed to git repo; I updated commit sha1. New commits:
d7fab0b  Merge branch 'develop' into t/18036/public/18036

comment:38 Changed 11 months ago by
 Cc ghkliem ghmwageringel added
 Milestone changed from sage6.6 to sage9.2
comment:39 Changed 10 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:40 Changed 8 months ago by
 Branch changed from public/18036 to u/mmezzarobba/18036QQi
 Commit changed from d7fab0b6a8628301543ed7bfae70380e1d6fa714 to 2657cd768a920d339cc7bc9d2b4dd88cf3862d8d
 Description modified (diff)
Time to reboot this ticket! (Still a bit of a work in progress, comments welcome.)
comment:41 Changed 8 months ago by
 Commit changed from 2657cd768a920d339cc7bc9d2b4dd88cf3862d8d to 4d2df78446d3b3fcadde1a8b9431451a8bc537fe
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
2d8979c  #18036 change global I to an element of ℚ[i]

953aa1e  #18036 add SR.I() for conversionless access to the symbolic I

68ac2e7  #18036 add a dedicated element class for ℚ[i]

6409f1e  #18026 explicit pushout(InfinityRing, *) > SR

2c68f5c  #18036 fix imports of I from symbolic.all

544c141  #18036 update some doctests using I.pyobject()

8a1ffe9  #18036 I → SR(I) in some doctests

79d4513  #18036 minor code adaptations in rings/

597ccc4  #18036 update some doctests in rings/

4d2df78  #18036 miscellaneous doctest updates

comment:42 Changed 8 months ago by
 Status changed from new to needs_review
comment:43 Changed 8 months ago by
 Commit changed from 4d2df78446d3b3fcadde1a8b9431451a8bc537fe to e47cd5bff02a0ca5072cc2ee79cfdf42f0b4ca6c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
92254a3  #18036 fix imports of I from symbolic.all

4d16638  #18036 update some doctests using I.pyobject()

f6f4bba  #18036 I → SR(I) in some doctests

fb2cbf0  #18036 minor code adaptations in rings/

272137a  #18036 update some doctests in rings/

e47cd5b  #18036 miscellaneous doctest updates

comment:44 followup: ↓ 46 Changed 8 months ago by
I can't speak to all the small changes that impact number field users, but I really like how atomic your commits are, well explained, and overall you seem to have a good approach. And if the doctests still pass, that might be a good sign in and of itself (though obviously shouldn't be added to an rc at this late date). Can anyone think of any other "extra" methods like real_part
that should be added in class NumberFieldElement_gaussian
?
comment:45 Changed 8 months ago by
 Commit changed from e47cd5bff02a0ca5072cc2ee79cfdf42f0b4ca6c to c4d4fa750c0220b3c5b56d7bdd05651498dd3614
comment:46 in reply to: ↑ 44 Changed 8 months ago by
Replying to kcrisman:
I can't speak to all the small changes that impact number field users,
It should not have much impact, except maybe for the conversion to pari which is overridden to return a pari object of the form a+I*b instead of a element of a quotient ring. I don't know pari well enough to be able to tell if that can be an issue.
but I really like how atomic your commits are, well explained, and overall you seem to have a good approach.
Thank you!
And if the doctests still pass, that might be a good sign in and of itself (though obviously shouldn't be added to an rc at this late date).
No. Ideally, I would like it to be merged early in the next release cycle, so that we have time to sort out any issues.
Can anyone think of any other "extra" methods like
real_part
that should be added inclass NumberFieldElement_gaussian
?
There was a discussion above about possibly supporting a significant subset of the methods of symbolic expressions, including elementary and special functions. I am reluctant to do that, because we will never achieve perfect compatibility, and integers and rationals already have their own API (so that users already need explicit conversions to SR in many cases if they want to use methods of symbolic expressions). But there may still be other methods that make sense to add.
comment:47 Changed 8 months ago by
 Commit changed from c4d4fa750c0220b3c5b56d7bdd05651498dd3614 to 3ab8aa6a01ee9c4f6e4040b3f7817f3d8a81f1fa
comment:48 Changed 8 months ago by
 Commit changed from 3ab8aa6a01ee9c4f6e4040b3f7817f3d8a81f1fa to b724b83b07333d97069088cf5c79c6dbdb8a0601
comment:49 Changed 8 months ago by
 Commit changed from b724b83b07333d97069088cf5c79c6dbdb8a0601 to d46c7abb5299ba48bc3f8a2a627522bd47187dbc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d46c7ab  #18036 miscellaneous doctest updates

comment:50 Changed 8 months ago by
 Commit changed from d46c7abb5299ba48bc3f8a2a627522bd47187dbc to 59d4e9870453e0ccf2097f525ea20243da7a18a7
comment:51 Changed 8 months ago by
 Commit changed from 59d4e9870453e0ccf2097f525ea20243da7a18a7 to 423506edf89295469a2c75cb0a41e206a98e9839
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
423506e  #18036 miscellaneous doctest updates

comment:52 Changed 8 months ago by
 Commit changed from 423506edf89295469a2c75cb0a41e206a98e9839 to 983a0c73400e57834d0a0f45d136c944a78a5b55
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0604abe  #18036 explicit pushout(InfinityRing, *) > SR

3a6f981  #18036 fix imports of I from symbolic.all

6c7cc80  #18036 fix type test in EllipticCurve_rational_field.eval_modular_form()

ab91938  #18036 minor code adaptations in rings/

5280659  #18036 doctest updates: I.pyobject()

f48d481  #18036 doctest updates: I → SR(I)

715bcf1  #18036 doctest updates: parigp interfaces

c78746e  #18036 doctest updates: modular forms

983a0c7  #18036 doctest updates: misc benign changes

comment:53 Changed 8 months ago by
Okay, the branch is now stable as far as I am concerned, and all tests to pass. It wouldn't be a bad time to start reviewing it if anyone is interested!
comment:54 Changed 8 months ago by
impressive :)
comment:55 Changed 8 months ago by
I am annoyed that you had to introduce a specific class NumberFieldElement_gaussian
. In the long run do you intend to put more in it or to remove it? For the sake of this ticket, I would
 implement
_symbolic_
,real_part
,imag_part
onNumberFieldElement_quadratic
(taking care of the embedding). Note that this would be desirable for more general number fields but more complicated (eg which symbolic representation? And number fields are generally not stable under taking real part.)  explicitely deprecate
log
So that NumberFieldElement_gaussian
would just be a transition class.
comment:56 followup: ↓ 57 Changed 8 months ago by
Also, the default embedding is not ideal
sage: I.parent().coerce_embedding() Generic morphism: From: Number Field in I with defining polynomial x^2 + 1 with I = 1*I To: Complex Lazy Field Defn: I > 1*I
Could we set it to QQbar
?
comment:57 in reply to: ↑ 56 ; followup: ↓ 58 Changed 8 months ago by
Replying to vdelecroix:
I am annoyed that you had to introduce a specific class
NumberFieldElement_gaussian
. In the long run do you intend to put more in it or to remove it?
To put more in it, probably.
Replying to vdelecroix:
Also, the default embedding is not ideal
sage: I.parent().coerce_embedding() Generic morphism: From: Number Field in I with defining polynomial x^2 + 1 with I = 1*I To: Complex Lazy Field Defn: I > 1*ICould we set it to
QQbar
?
I was thinking of doing that later, in #12715. But I will try to see if doing it now just for ℚ[i] breaks anything.
comment:58 in reply to: ↑ 57 Changed 8 months ago by
Replying to mmezzarobba:
Could we set it to
QQbar
?I was thinking of doing that later, in #12715. But I will try to see if doing it now just for ℚ[i] breaks anything.
That creates import loops which may require to change the order of imports in sage.all significantly: the initialization of QQbar in sage.all currently comes very late, but the change would require moving it before that of pynac, which is done much earlier. I don't have the courage to try to untangle that mess at the moment.
comment:59 Changed 8 months ago by
 Commit changed from 983a0c73400e57834d0a0f45d136c944a78a5b55 to feb4333de3597611e7fbbfce800dcab2ee67aba6
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
549b73b  #18036 add a dedicated element class for ℚ[i]

357c7e3  #18036 explicit pushout(InfinityRing, *) > SR

6b63640  #18036 fix imports of I from symbolic.all

2d3bf87  #18036 fix type test in EllipticCurve_rational_field.eval_modular_form()

a9937ef  #18036 minor code adaptations in rings/

b6e1117  #18036 doctest updates: I.pyobject()

e8a8ce9  #18036 doctest updates: I → SR(I)

860bbfb  #18036 doctest updates: parigp interfaces

955712d  #18036 doctest updates: modular forms

feb4333  #18036 doctest updates: misc benign changes

comment:60 Changed 8 months ago by
(trivial rebase on final 9.2)
comment:61 Changed 8 months ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
Good! I hope it will get smoothly into the next beta.
comment:62 Changed 8 months ago by
Thanks a lot!
comment:64 Changed 8 months ago by
Thanks Volker. Could you merge it early in the next beta? It is likely to create merge conflicts because touching a lot of files.
comment:65 Changed 8 months ago by
 Commit changed from feb4333de3597611e7fbbfce800dcab2ee67aba6 to f885b3d5f8d6432537d1d07afc75382a4a0e107d
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9976c5e  #18036 add a dedicated element class for ℚ[i]

3575380  #18036 explicit pushout(InfinityRing, *) > SR

8d92af6  #18036 fix imports of I from symbolic.all

f4b41f6  #18036 fix type test in EllipticCurve_rational_field.eval_modular_form()

78a69e0  #18036 minor code adaptations in rings/

f430a42  #18036 doctest updates: I.pyobject()

68e0860  #18036 doctest updates: I → SR(I)

81c3a79  #18036 doctest updates: parigp interfaces

73185af  #18036 doctest updates: modular forms

f885b3d  #18036 doctest updates: misc benign changes

comment:66 Changed 8 months ago by
 Status changed from needs_work to needs_review
comment:67 Changed 8 months ago by
 Status changed from needs_review to positive_review
Rebase was trivial, tests still pass.
comment:68 followup: ↓ 69 Changed 8 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:69 in reply to: ↑ 68 Changed 8 months ago by
comment:70 Changed 8 months ago by
 Commit changed from f885b3d5f8d6432537d1d07afc75382a4a0e107d to 10c5abf85b9c0e18b2bb635d2a5f9ed7f3fc0096
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
11b239c  #18036 add a dedicated element class for ℚ[i]

5ed00bc  #18036 explicit pushout(InfinityRing, *) > SR

c9b08f6  #18036 fix imports of I from symbolic.all

11be379  #18036 fix type test in EllipticCurve_rational_field.eval_modular_form()

8125051  #18036 minor code adaptations in rings/

e5f9325  #18036 doctest updates: I.pyobject()

ed58a73  #18036 doctest updates: I → SR(I)

fe14b1a  #18036 doctest updates: parigp interfaces

671eed0  #18036 doctest updates: modular forms

10c5abf  #18036 doctest updates: misc benign changes

comment:71 Changed 8 months ago by
 Status changed from needs_work to needs_review
comment:72 Changed 7 months ago by
 Status changed from needs_review to positive_review
comment:73 Changed 7 months ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:74 Changed 7 months ago by
could be because of
+ An element of ℚ[i].
comment:75 Changed 7 months ago by
 Commit changed from 10c5abf85b9c0e18b2bb635d2a5f9ed7f3fc0096 to 54a34a7443d373e678c5461e5205ef2cdd7470b1
Branch pushed to git repo; I updated commit sha1. New commits:
54a34a7  #18036 fix docstring

comment:76 Changed 7 months ago by
Thanks Volker and Frédéric.
comment:77 Changed 7 months ago by
 Status changed from needs_work to positive_review
comment:78 Changed 7 months ago by
 Branch changed from u/mmezzarobba/18036QQi to 54a34a7443d373e678c5461e5205ef2cdd7470b1
 Resolution set to fixed
 Status changed from positive_review to closed
I'm not so sure it should. Which quadratic field is the appropriate one? There are many, distinguishable by the name of their generator (that would be 'I') for this one, but also by their specified embeddings, and it's not clear which one to choose.
Is there an argument for doing this? Ticket #17860 referenced in the description makes no mention of it. I'd imagine there might be evaluation reasons that might make it attractive. Perhaps those also give an indication of which quadratic field would be the appropriate one.