New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specialized class for real algebraic numbers #669

Merged
merged 3 commits into from Aug 11, 2018

Conversation

2 participants
@skirpichev
Copy link
Collaborator

skirpichev commented Aug 5, 2018

For elements there are define usual mathematical comparison ops (undefined for generic AlgebraicElement). RealAlgebraicElement support __abs__ magic method.

Also, this pull request implements RootOf.refine method.

@skirpichev skirpichev added this to the 0.10 milestone Aug 5, 2018

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 5, 2018

This looks good though I did not check the coding details. I had to look more closely into the automatic creation of RealAlgebraicFields but I now think that I understand the way it works. It is surely practical.

@skirpichev skirpichev force-pushed the skirpichev:real-algebraics branch 2 times, most recently from ec14c87 to 7d0b748 Aug 5, 2018

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 5, 2018

I had to look more closely into the automatic creation of RealAlgebraicFields but I now think that I understand the way it works.

There is a specialized cache version, like used in rings.py, fields.py and modularinteger.py (so, it's more-or-less consistent across the codebase). Probably, in future I'll replace this stuff with using standard Diofant's caching abilities (cacheit and so on), but right now it seems more hard then I expected.

I'm more concerned about mathematical stuff in ext_root property (this is a RootOf object, representing the primitive element, ext) and _cmp helper method (which implements comparison ops).

ext_root implemented for real algebraic fields so far, which meets practical needs.

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 5, 2018

What is the expected type of self.ext to make the comparison on line 167 meaningful?

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 5, 2018

An Expr instance, like sqrt(2).

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 6, 2018

It is hard to see, in general, that an Expr, ext, is real (even if it is). ext.is_real will often return None. I see that you play it safe and define the field as an AlgebraicField in that case. That would make it plausible that the comparison on line 167 won't raise an exception as it is only attempted when self.ext.is_real is true.

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 6, 2018

ext.is_real will often return None. I see that you play it safe and define the field as an AlgebraicField in that case.

Perhaps, I should be more careful here and take into account this ternary logic. In sense that we should raise an exception in constructor, instead of using AlgebraicField as a fallback for None.

However, It's hard to find a case, where ext.is_real would return None. Perhaps, the only case would be if minpoly is Poly(x, x), when ext in fact is a zero. Maybe I should check this pathological scenario in the AlgebraicField constructor.

The point of ext_root is to select a single root of minpoly for extension ext. Another variant for this procedure is testing difference ext - r for all minpoly roots r. If that difference can be computed with precision (and is nonzero) - this r is invalid for our purposes. But I'm afraid that the only way to test this in infallible way - compute minpoly(ext - r).

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 6, 2018

It's hard to find a case, where ext.is_real would return None.

They are not quite trivial to find, but the roots of a cubic polynomial tend to be such in the Casus Irreducibilis case. This is one example that I found in test_roots_cubic():
eq = -x**3 + 2*x**2 + 3*x - 2.

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 6, 2018

This is one example that I found in test_roots_cubic(): eq = -x**3 + 2*x**2 + 3*x - 2.

Indeed. But provided above variant with difference testing seems to be working here:

In [1]: eq = -x**3 + 2*x**2 + 3*x - 2

In [2]: rs = roots(eq, multiple=True)

In [3]: rs[0].is_real

In [4]: (rs[0] - RootOf(eq, 0)).n(2)
Out[4]: 4.2 - 0.e-7⋅ⅈ

In [5]: (rs[0] - RootOf(eq, 1)).n(2)
Out[5]: 2.3 - 0.e-7⋅ⅈ

In [6]: (rs[0] - RootOf(eq, 2)).n(2)
---------------------------------------------------------------------------
PrecisionExhausted                        Traceback (most recent call last)
...

@skirpichev skirpichev force-pushed the skirpichev:real-algebraics branch from 9a982a3 to b4fd3e3 Aug 6, 2018

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 7, 2018

I used more careful testing of is_real with fallback in case ext.is_real is None to the full-blown minimal_polynomial-based method, which determine an appropriate root of minpoly.

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 7, 2018

if not minimal_polynomial(ext - r)(0):

In which form will r be here?

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 7, 2018

@skirpichev skirpichev force-pushed the skirpichev:real-algebraics branch 2 times, most recently from b98ee31 to 518cf44 Aug 8, 2018

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 10, 2018

I'm going to merge this, unless you have better ideas how to select an appropriate minpoly root for the primitive element.

@jksuom

This comment has been minimized.

Copy link
Contributor

jksuom commented Aug 10, 2018

I cannot think of a way to avoid numerical evaluation, in one way or another. There may be several ways of organizing the selection, but this is probably as good as any. Perhaps practical use will bring forth new ideas.

skirpichev added some commits Aug 5, 2018

domains: Add RealAlgebraicField subclass
This commit also reimplement rich comparisons for AlgebraicField.dtype's.
These operations defined now only for RealAlgebraicElement's.

We also compute the RootOf representation (ext_root) of the primitive
element if assumption test ext.is_real return None.  To select
appropriate root we compute minimal_polynomial of the difference
ext and the candidate root.

@skirpichev skirpichev force-pushed the skirpichev:real-algebraics branch from 518cf44 to 3a1f5fe Aug 10, 2018

@skirpichev skirpichev merged commit ec7a78e into diofant:master Aug 11, 2018

3 checks passed

codecov/patch 100% of diff hit (target 97%)
Details
codecov/project Absolute coverage decreased by -<1% but relative coverage increased by +3% compared to ded7efa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skirpichev skirpichev deleted the skirpichev:real-algebraics branch Aug 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment