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

make better decisions for minpoly based on domain #14494

Open
smichr opened this Issue Mar 13, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@smichr
Copy link
Member

smichr commented Mar 13, 2018

#14382 gives

>>> minimal_polynomial(I, x, domain=QQ.algebraic_field(I))
x - I

instead of

x**2 + 1

but @jksuom noted:
private methods like _minpoly_exp(ex, x) ... ignore the dom argument.

@skirpichev suggested:
"...you could "sanitize" output of minpoly helpers: factorize one over specified dom (if it's badly supported) and select appropriate factor with _choose_factor()." Also, "[A more] systematic workaround may be using compose=False algorithm (currently, it doesn't support anything except Q) for algebraic extensions."

example


In [11]: minimal_polynomial(exp(I*pi/3), x, domain=QQ.algebraic_field(sqrt(3)*I))
Out[11]: 
 2        
x  - x + 1

In [12]: factor(_, extension=sqrt(3)*I)
Out[12]: 
⎛          ___  ⎞ ⎛          ___  ⎞
⎜    1   ╲╱ 3 ⋅ⅈ⎟ ⎜    1   ╲╱ 3 ⋅ⅈ⎟
⎜x - ─ - ───────⎟⋅⎜x - ─ + ───────⎟
⎝    2      2   ⎠ ⎝    2      2   ⎠
@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Mar 13, 2018

I would try adding

_, factors = factor_list(res, x, domain=dom)
res = _choose_factor(factors, x, ex)

before the final return res in _minpoly_compose. Unit tests should then be added.

@abaokar-07

This comment has been minimized.

Copy link
Contributor

abaokar-07 commented Mar 13, 2018

I would try adding

Still the output is x - I after the following change:

_, factors = factor_list(res, x, domain=dom)
res = _choose_factor(factors, x, ex)
return res
@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Mar 13, 2018

@abaokar-07, I hope someone here will "invent" the change, that "fix" this output ;)

@abaokar-07

This comment has been minimized.

Copy link
Contributor

abaokar-07 commented Mar 13, 2018

@skirpichev yeah sure, I just tried what @jksuom suggested and reported here

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Mar 13, 2018

yeah sure

Oh, I hope - no.

That was a joke, actually. Please try to understand issue, before "fixing" it. I.e. what is a correct output for the problem, what was incorrect and so on.

@abaokar-07

This comment has been minimized.

Copy link
Contributor

abaokar-07 commented Mar 13, 2018

That was a joke, actually.

Yeah, I got that :D . And I'll keep in mind your suggestion, however, I tried to understand the issue, maybe I have misunderstood it

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 14, 2018

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