#7160: comparison with quadratic number field elements needs work
-----------------------------+----------------------------------------------
Reporter: mhansen | Owner: davidloeffler
Type: defect | Status: needs_review
Priority: major | Milestone: sage-4.8
Component: number fields | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author: Mike Hansen
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Comment(by was):
1. Typo: " comparision" in
sage/rings/number_field/number_field_element_quadratic.pyx.
2. This code raises a big red flag for me:
{{{
1704 from sage.rings.all import RR
1705 r = RR(right)
1706 l = RR(embedding(left))
1707 return cmp(l, r)
}}}
My concern is that you're comparing to double (=53 bits) precision, which
is totally arbitrary. What if there is an embedding and cmp(left, right)
requires 54 bits of precision to sort out? I could probably with some
work construct a nasty example of this, where the above code just gives
totally the wrong answer. I guess we need to (1) compute to precision of
the embedding, and (2) if cmp(l,r)==0, then check something more?
3. Where is "self._element_class" actually used? I guess by the coercion
model, but it's hard to see how from this patch, exactly.
4. I feel like this is too much code that gets around a fundamental
problem or bug in number field elements in the case of the reported
problem, but leaves the underlying bug unfixed. Note that *before*
applying your patch:
{{{
sage: bool(I^2 < 0)
True
sage: (I^2).pyobject() < 0
False
}}}
It seems to me that the output of both lines above should be the same,
right, but I bet pynac is evaluating the first comparison and not even
going to the pyobject level. Similarly:
{{{
sage: bool(I^2 < SR(0))
True
sage: (I^2).pyobject() < SR(0).pyobject()
False
}}}
This is because in Sage we currently have:
{{{
sage: K.<I> = QuadraticField(-1)
sage: I^2 < 0
False
sage: I^2
-1
}}}
This is, as Cremona pointed out, due to the arbitrary lexicographic
ordering rather than using the one we get on the intersection of K and R
inside embedding(K).
So... it seems to me that the real problem is that the arbitrary ordering
is lame. What you've done is implemented a natural fix in one very, very
special case. Maybe that's the intention, and maybe you had something
more general before? I don't know, since you changed the patch.
I'm guessing your more general patch changed comparison for all elements
to:
(1) check if there is an embedding of the parent(s), and (2) if so, use
that to induce an embedding on *the* real subfield of the field(s). That
would be natural.
So all of the above, is just me understanding why you are doing what
you're doing. It might make sense to add something like this to the
documentation.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7160#comment:7>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
