Skip to content
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

factor with extension=True fails for rational expression #24346

Open
oscarbenjamin opened this issue Dec 4, 2022 · 4 comments
Open

factor with extension=True fails for rational expression #24346

oscarbenjamin opened this issue Dec 4, 2022 · 4 comments
Labels

Comments

@oscarbenjamin
Copy link
Contributor

Here factor with extension=True should use Q(sqrt(2)) in which case this should factor and cancel:

In [1]: factor((x**2 - 2)/(x + sqrt(2)), extension=True)
Out[1]: 
 2    
x  - 2
──────
x +2

It succeeds if the extension is specified explicitly:

In [2]: factor((x**2 - 2)/(x + sqrt(2)), extension=sqrt(2))
Out[2]: x -2
@oscarbenjamin
Copy link
Contributor Author

The problem is that factor does not try to find a common domain for the numerator and denominator:

sympy/sympy/polys/polytools.py

Lines 6011 to 6033 in 900caa3

args = [i._eval_factor() if hasattr(i, '_eval_factor') else i
for i in Mul.make_args(expr)]
for arg in args:
if arg.is_Number or (isinstance(arg, Expr) and pure_complex(arg)):
coeff *= arg
continue
elif arg.is_Pow and arg.base != S.Exp1:
base, exp = arg.args
if base.is_Number and exp.is_Number:
coeff *= arg
continue
if base.is_Number:
factors.append((base, exp))
continue
else:
base, exp = arg, S.One
try:
poly, _ = _poly_from_expr(base, opt)
except PolificationFailed as exc:
factors.append((exc.expr, exp))
else:
func = getattr(poly, method + '_list')

@oscarbenjamin
Copy link
Contributor Author

Maybe here is more the point:

sympy/sympy/polys/polytools.py

Lines 6080 to 6094 in 900caa3

def _generic_factor_list(expr, gens, args, method):
"""Helper function for :func:`sqf_list` and :func:`factor_list`. """
options.allowed_flags(args, ['frac', 'polys'])
opt = options.build_options(gens, args)
expr = sympify(expr)
if isinstance(expr, (Expr, Poly)):
if isinstance(expr, Poly):
numer, denom = expr, 1
else:
numer, denom = together(expr).as_numer_denom()
cp, fp = _symbolic_factor_list(numer, opt, method)
cq, fq = _symbolic_factor_list(denom, opt, method)

@oscarbenjamin
Copy link
Contributor Author

Looking at this I also see this bug:

In [18]: Integral(2*x, x, y)
Out[18]: 
⌠ ⌠          
⎮ ⎮ 2x dx dy
⌡ ⌡          

In [19]: Integral(2*x, x, y).factor()
Out[19]: 
    ⌠     
2y⋅⎮ x dx

Here I don't think that factor should shortcut to evaluate the integral. This should give:

In [20]: 2*Integral(x, x)*Integral(1, y)
Out[20]: 
  ⌠      ⌠     
2⋅⎮ 1 dy⋅⎮ x dx
  ⌡      ⌡  

This is analogous to

In [21]: Integral(2*x*y, x, y).factor()
Out[21]: 
  ⌠      ⌠     
2⋅⎮ x dx⋅⎮ y dy
  ⌡      ⌡  

This happens because of doit here:

if not summand.has(self.variables[-1]):
return self.func(1, [self.limits[-1]]).doit()*summand

@oscarbenjamin
Copy link
Contributor Author

I also wonder if the last example here should pull the factors out of an integral:

In [4]: i = Integral(2*x, x, y)

In [5]: i
Out[5]: 
⌠ ⌠          
⎮ ⎮ 2x dx dy
⌡ ⌡          

In [6]: factor(i)
Out[6]: 
    ⌠     
2y⋅⎮ x dxIn [7]: factor(1 + i)
Out[7]: 
⌠ ⌠              
⎮ ⎮ 2x dx dy + 1
⌡ ⌡  

skirpichev added a commit to skirpichev/diofant that referenced this issue Dec 7, 2022
skirpichev added a commit to skirpichev/diofant that referenced this issue Dec 7, 2022
skirpichev added a commit to skirpichev/diofant that referenced this issue Dec 8, 2022
skirpichev added a commit to skirpichev/diofant that referenced this issue Dec 8, 2022
skirpichev added a commit to skirpichev/diofant that referenced this issue Dec 12, 2022
skirpichev added a commit to diofant/diofant that referenced this issue Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant