-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
[WIP] Polynomial System Factorization Methods (continued) #27303
base: master
Are you sure you want to change the base?
[WIP] Polynomial System Factorization Methods (continued) #27303
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.14. Click here to see the pull request description that was parsed.
|
…tion systems Added a new function `factor_system` to sympy.polys.polytools that finds all possible combinations of factors of equations of a polynomial system that could simultaneously all be zero to satisfy a system of equations.
305b486
to
171bd1e
Compare
Alright, continuing where we left off: The last review suggested the following changes:
>>> eqs = [a * (x - 1) * (y - 1), b * (x - 2) * (y - 1) * (y - 2)]
>>> factor_system_bool(eqs, [x, y])
(Eq(y - 1, 0) | (Eq(a, 0) & Eq(b, 0)) |((Eq(a, 0) | Eq(x - 1, 0)) & (Eq(b, 0) | Eq(x - 2, 0))) | ((Eq(a, 0) | Eq(x - 1, 0)) & (Eq(b, 0) | Eq(y - 2, 0)))) Here it was pointed out that the case
|
Also, can someone please explain why the CI tests here are failing when for the same code in the parent PR they didn't. It was unexpected for me. How may I fix it ? |
I think the failed test is basically the same issue as gh-20456. |
I think that all of these functions should be moved to I also think that All of these functions should be added to the docs so we should make sure they are added there somewhere. The docstrings should be simplified with the use of I don't like the fact that |
Does something like this suffice ? : def factor_poly_system(eqs, *gens, **kwargs):
systems, conds = factor_system_cond(eqs, *gens, **kwargs)
# Converting systems to ordered lists
ordered_systems = []
for system in systems:
# Extracting expresions from (factor, conditions) pairs and sort them here only
exprs = [f.as_expr() for f, c in system]
ordered_systems.append(sorted(exprs, key=str))
# Sort the systems themselves
ordered_systems.sort(key=lambda x: [str(expr) for expr in x])
# Make solvability condition
if not systems:
solvability = False if conds else True
else:
# Base solvability on the constant conditions
solvability = True if not conds else And(*[Eq(c.as_expr(), 0) for c in conds])
return ordered_systems, solvability |
I think it can just be: def factor_system(eqs, *gens, **kwargs):
systems, conds = factor_system_cond(eqs, *gens, **kwargs)
systems = [{f.as_expr() for f, c in system} for system in systems]
return systems, And(*conds) If we are going to sort the output then I would prefer to do it in the base routine We shouldn't use |
This PR is a continuation of work from here. For context please refer to the discussion in the parent PR.
Release Notes