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

Poly constructor uses domain EX when it's not necessary #14337

Open
normalhuman opened this Issue Feb 25, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@normalhuman
Copy link
Contributor

normalhuman commented Feb 25, 2018

I understand that polynomial manipulations with domain EX are slow. So it came as a surprise that this domain is used by polynomial constructor when a simpler one could do. Consider this example:

>>> var('a x')
>>> poly(x**2 + sqrt(a), x)
Poly(x**2 + sqrt(a), x, domain='ZZ[sqrt(a)]')
>>> poly(x**2 + sqrt(2), x)
Poly(x**2 + sqrt(2), x, domain='EX')

If ZZ[sqrt(a)] is automatically used to handle sqrt(a), why isn't ZZ[sqrt(2)] used to handle sqrt(2)?

Similarly,

>>> poly(x**2 + I, x)
Poly(x**2 + I, x, domain='EX')

This is a polynomial over Gaussian integers, the domain should be ZZ[I].

@normalhuman normalhuman added the polys label Feb 25, 2018

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Feb 26, 2018

This is an old, unresolved issue, cf. #5428.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Feb 26, 2018

@jksuom reply catch major part of mine, but there are several minor issues.

This is a polynomial over Gaussian integers, the domain should be ZZ[I].

  1. Should it be ZZ[I], not QQ(I)?

  2. In Sympy, ZZ[I] is a polynomial ring over ZZ with generator I (ZZ.poly_ring(I)), which doesn't assume any algebraic relations among generators. (So, technically, there is no Gaussian integers yet.) Same story for fields (e.g. ZZ.frac_field(I)). But there is a specialized version for algebraic number fields, namely QQ.algebraic_field(I), which does assume I**2 == - 1.

    Perhaps, FracField's constructor should transform some cases (e.g. QQ.frac_field(I)) to more specialized versions.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 26, 2018

Issues of using algebraic fields automatically aside, it does seem odd that sqrt(a) uses a polynomial ring and sqrt(2) uses EX. Neither is particularly good from a mathematical point of view: ZZ[sqrt(a)] does not know that (sqrt(a))**2 - a) is 0. EX knows this but only because it simplifies automatically (it won't work for more complicated algebraic expressions.

I believe performance of the algebraic domains is still an issue. Otherwise they should be used by default. By the way I'm not convinced that EX should be slower than a polynomial ring. Has it been benchmarked? Are simplifications attempted on the EX coefficients automatically?

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Feb 26, 2018

Otherwise they should be used by default.

I did this for Diofant (diofant/diofant#478). Not too hard, but there are several places where extension option should be disabled to pass CI tests. In fact, working with coefficients over algebraic field is not slow. That is slow - a construction of suitable algebraic number fields.

Are simplifications attempted on the EX coefficients automatically?

If you ever read the code - it should be clear for you, that simplify() (actually, cancel()) called after almost any algebraic operation with Expression() (element of EX). So yes, it attempted, for sure.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Feb 26, 2018

Are simplifications attempted on the EX coefficients automatically?

This is not done. I think that is the principal issue with EX. I have seen computations with huge expressions whose value (after simplification) is 0.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 26, 2018

Calling cancel on an EX expression would be no different from just having the coefficients be from a fraction domain in the first place, except slower because the domain is recreated each time. If that's the case, I can see why EX would be slower. However if it isn't the case it seems it should be faster.

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