allow conditions being evaluated in Solve #918

BlankShrimp · 2023-09-10T06:51:44Z

For expressions like Solve[x+1==0 && x<1, x], Mathics3 unexpectedly blocks the ability to handle the conjuncted logics when determine whether the expression is well-formed. This PR unlocks this ability by putting off the check after all eqs were processed, and using sympy.And().subs() to filter solutions using conjuncted logics.

mmatera · 2023-09-10T14:14:03Z

@BlankShrimp thanks for looking at this. Could you please add some examples to the doctests?

BlankShrimp · 2023-09-11T15:26:44Z

@BlankShrimp thanks for looking at this. Could you please add some examples to the doctests?

Done. A few cases have been added.

mmatera · 2023-09-11T22:02:02Z

mathics/builtin/numbers/calculus.py

@@ -2254,8 +2255,7 @@ def eval(self, eqs, vars, evaluation: Evaluation):
            elif eq is SymbolFalse:
                return ListExpression()
            elif not eq.has_form("Equal", 2):
-                evaluation.message("Solve", "eqf", eqs)
-                return
+                sympy_conditions.append(eq.to_sympy())


It must be checked that eq is a condition, and has a sympy form.

The test Solve[{x^2==4 && x < 0 && x > -4},{x}] fails because here the first argument is treated as a single condition x^2==4 && x < 0 && x > -4 without any equation. Conjunctions should be decomposed to work.

It must be checked that eq is a condition,

It's my bad, I'll try to investigate the way to do this.

and has a sympy form.

I noticed this issue while writing these lines, but I think this problem is out of the scope of this contribution. I have little knowledge over Mathics, so if I said anything wrong please do point out. My argument is that anything that to_sympy() can't translate to a sympy expression shouldn't be considered as valid. In this case, the exception would catch the error and fail the expression.

The test Solve[{x^2==4 && x < 0 && x > -4},{x}] fails because here the first argument is treated as a single condition x^2==4 && x < 0 && x > -4 without any equation. Conjunctions should be decomposed to work.

Agreed. Actually, I noticed this issue too while writing & testing, and put this test case intentionally looking for your opinion. If you could try Solve[a^2 - 3a + 2 == 0 && a > 1, a] with my code, you will find it works perfectly. I think this is a parsing issue, the parser can't always decompose conjunctions correctly. In fact, I also noticed that the parser can't handle things like <some eqs> && <condition1> || <condition2> neither, which produces a non-sympy object. In any casaes, I think this problem is also out of the scope of this contribution.

@BlankShrimp the parser can't always decompose conjunctions correctly.

Aha, no doubt I have little knowledge over Mathics. There's nothing wrong with Mathics, it's my job to decompose the conjunction inside solve.eval(). Wait for my code.

Agreed. Actually, I noticed this issue too while writing & testing, and put this test case intentionally looking for your opinion. If you could try Solve[a^2 - 3a + 2 == 0 && a > 1, a] with my code, you will find it works perfectly. I think this is a parsing issue, the parser can't always decompose conjunctions correctly. In fact, I also noticed that the parser can't handle things like <some eqs> && <condition1> || <condition2> neither, which produces a non-sympy object. In any casaes, I think this problem is also out of the scope of this contribution.

@BlankShrimp : Indeed, your example works, but Solve[{x^2==4 && x<0}, x] (the conjuction inside a list) doesn't. To do it right, the interpretation of the first argument should be done recursively.

This could be a way to do that: replace the loop over eq_list by

def process_equation(eq): if eq is SymbolTrue: pass elif eq.has_form("And", None): for sub_eq in eq.elements: process_equation(sub_eq) elif eq.has_form("Equal", 2): left, right = eq.elements left = left.to_sympy() right = right.to_sympy() if left is None or right is None: return eq = left - right eq = sympy.together(eq) eq = sympy.cancel(eq) sympy_eqs.append(eq) numer, denom = eq.as_numer_denom() sympy_denoms.append(denom) else: cond_sympy = eq.to_sympy() if cond_sympy is None: evaluation.message("Solve", "eqf", eqs) raise ValueError(str(eq)) sympy_conditions.append(cond_sympy) return None for eq in eq_list: if eq is SymbolFalse: return ListExpression() try: process_equation(eq) except ValueError: return None

With this change, and one on the test on the existence of equations (if not sympy_eqs -> if not sympy_eqs and sympy_conditions) all the tests pass.

On first look, it feels like the purpose of the above is to collect sympy conditions. And this feels like something that might come up other places as well, right? Perhaps this should be put inside mathics.core.convert.sympy then

Small thing: instead of if ... pass , if ... return None is more direct.

mmatera · 2023-09-11T22:05:09Z

mathics/builtin/numbers/calculus.py

@@ -2269,6 +2269,10 @@ def eval(self, eqs, vars, evaluation: Evaluation):
                numer, denom = eq.as_numer_denom()
                sympy_denoms.append(denom)

+        if not sympy_eqs:


This breaks an existing test: Solve[True,x] should return {{}} without warnings. On the other hand,

Solve[{x>5}, x]

produces the message:

Solve::fulldim: The solution set contains a full-dimensional component; use Reduce for complete solution information.

and

Solve[Q, x]

produces

Solve::naqs: Q is not a quantified system of equations and inequalities.

Oh god the warning issue was overlooked, I'll look into it.

mmatera · 2023-09-11T22:16:43Z

@BlankShrimp tests are not passing. Please check the comments.

mmatera · 2023-09-12T00:22:32Z

test/builtin/calculus/test_solve.py

+        ),
+        (
+            "Solve[{x^2==4 && x < 0 && x > -4},{x}]",
+            "{x->-2}",


In WMA, the output is "{{x -> -2}}"

mmatera · 2023-09-12T00:22:41Z

test/builtin/calculus/test_solve.py

@@ -42,6 +42,16 @@ def test_solve():
            "{x->1.51213}",
            "Issue #1235",
        ),
+        (
+            "Solve[{x^2==4 && x < 0},{x}]",
+            "{x->-2}",


In WMA, the output is "{{x -> -2}}"

rocky · 2023-09-14T05:21:58Z

I have been busy recently, so I haven't had a chance to look about or think about this in detail.

I will mention some things that you can't really be expected to know.

Originally, the builtin-function code (under the mathics.builtin module) had all sorts of code (in the U.S. the phrase we sometimes use is: "including the kitchen sink'). It had code related to implementation, boxing, formatting, testing, and more stuff.

In addition to making the code hard to read, this organization was/is slow too. If we ever have a hope of speeding this up, we have to pull out at least evaluation code to make it possible to serialize instructions rather that walk a M-Expression tree.

Basically this means functions like summand() and cut_var_demensions() need to be moved outside specific built in classes. The class organization is there to facilitate some sort of Mathics3 Rule pattern matching can be done on the M-Expression tree to select what particular Python class function to call evaluate.

Because of the bulk of existing code, we haven't done that fully. So that's why you find these functions still there.

The top-level question I have is whether this kind of thing is only used in Solve[] or whether there is a more general kind of thing going on to convert the Mathics3 expression to a SymPy expression.

If it is a more general thing then these functions would go in mathics.core.convert. If it is very specific to Solve or more generally the calculus-like functions. Then it would probably go into mathics.eval.calculus.

mmatera · 2023-09-17T18:01:29Z

I have been busy recently, so I haven't had a chance to look about or think about this in detail.

I will mention some things that you can't really be expected to know.

Originally, the builtin-function code (under the mathics.builtin module) had all sorts of code (in the U.S. the phrase we sometimes use is: "including the kitchen sink'). It had code related to implementation, boxing, formatting, testing, and more stuff.

In addition to making the code hard to read, this organization was/is slow too. If we ever have a hope of speeding this up, we have to pull out at least evaluation code to make it possible to serialize instructions rather that walk a M-Expression tree.

Basically this means functions like summand() and cut_var_demensions() need to be moved outside specific built in classes. The class organization is there to facilitate some sort of Mathics3 Rule pattern matching can be done on the M-Expression tree to select what particular Python class function to call evaluate.

Because of the bulk of existing code, we haven't done that fully. So that's why you find these functions still there.

The top-level question I have is whether this kind of thing is only used in Solve[] or whether there is a more general kind of thing going on to convert the Mathics3 expression to a SymPy expression.

If it is a more general thing then these functions would go in mathics.core.convert. If it is very specific to Solve or more generally the calculus-like functions. Then it would probably go into mathics.eval.calculus.

When @BlankShrimp finishes with this work, then I can move the code to eval / convert module.

BlankShrimp · 2023-09-20T07:12:52Z

If it is a more general thing then these functions would go in mathics.core.convert. If it is very specific to Solve or more generally the calculus-like functions. Then it would probably go into mathics.eval.calculus.

I'm currently looking at this. What's the difference between mathics.algorithm and mathics.eval? Like I can see simplify.py is in mathics.algorithm, where Solve[] and Simplify[] is kinda symmetricalto me.

rocky · 2023-09-20T08:53:56Z

What's the difference between mathics.algorithm and mathics.eval?

mathics.algorithm was found in Mathics 1.0. In my opinion, it should be reorganized into mathics.eval

In my view, things back in 1.0 were a little haphazard or whimsical and didn't follow rigid thinking. In some cases, terminology was just plain wrong.

Most of the time, things were in coded inside the builtin routine either inside a class there or as a module-level method. For mathics.algorithm, it seems that this was split off into a new place. This in my opinion was a step in the right direction in the sense that it reduces bulk in mathics.builtin. If you look at git commit history, you can probably get a better sense of what and why.

Here is the current thinking though....

What goes into the mathics.builtin module are Mathics3 built in functions, but we try to limit this to the top-level evaluation "Builtin class" class kinds of things. There is a fancy mechanism to locate and find which evaluation method to apply, based on definition lookup of symbols after transformation rule application.

In the current thinking, this is all that should be put in there. Rules for how to Box, or how to format to SVG, LaTeX, or common evaluation routines and their helpers should not be there. It even used to be that unit tests were put in the docstring in a non-standard encoding (not sphinx) format.

The organization of the modules inside the builtins follows (roughly) the "Guide "section organization of WMA. Where this gets off is that the WMA hierarchy is more DAG like than tree like. Or in other words a single WMA builtin can appear in more than one Guide section. So here we just have to pick one to place the builtin.

As I mentioned before, evaluation methods are there so that one has a chance of writing an interpreter which evaluates a linear sequence of instructions rather than walking an expression tree over possibly repeatedly.

The modules in the mathics.eval mirror the modules in mathics.builtin. There is a module called mathics.builtin.directories which corresponds to "Directories and Directory Operations" so there is a corresponding module called mathics.eval.directories . Here we just have module-level constants though.

Some of this is mentioned in https://mathics-development-guide.readthedocs.io/en/latest/extending/code-overview/python-modules.html and other places in that guide, but I see that I probably need to add more information.

From the discussion in PR #918 (#918 (comment)), I remembered that this was something pendant for a while, so I started moving these modules to follow the new organization of the code.

alternate old plain solve logic with a recursive one. Now Solve is capable with nested logics

1. added one missed line and a few catches 2. replace function type hint with 3.9 compatible style

1. bring domain check back into solve.eval, so that things like Abs() can be evaluated 2. create system symbols such as System`Reals for domain check 3. refactor, moving most logics out of Solve

mmatera · 2023-09-22T12:13:24Z

mathics/core/convert/sympy.py

@@ -5,6 +5,7 @@
 Conversion to SymPy is handled directly in BaseElement descendants.
 """

+from collections.abc import Iterable


The Iterable class that supports typing (Iterable[sympy.Symbol]) is in the typing module. You should also import Dict, List and Set for the following annotations.

typing.Iterable is deprecated. https://docs.python.org/3/library/typing.html#typing.Iterable

You are right. However, as we still support 3.8, we should do a conditional import.

mmatera · 2023-09-22T12:14:06Z

mathics/core/convert/sympy.py

+    return result
+
+
+def cut_dimension(evaluation, expressions: Union[Expression, list[Expression]], symbols: Iterable[sympy.Symbol]) -> set[sympy.Symbol]:


List[Expression] and Set[sympy.Symbol] (notice the caps)

list[...] is the better choice. https://peps.python.org/pep-0585/

Still, notice that at least until 3.8, you should use the capitalized name (List[...])

mmatera · 2023-09-22T12:16:00Z

mathics/builtin/numbers/calculus.py

    SymbolRule,
    SymbolSequence,
    SymbolSeries,
    SymbolSeriesData,
    SymbolSimplify,
    SymbolUndefined,
 )
+from mathics.eval.calculus import solve_sympy


mathics.eval.numbers.calculus.solvers and the module was not added to the branch.

mmatera · 2023-09-22T12:26:39Z

mathics/builtin/numbers/calculus.py

@@ -664,8 +668,8 @@ def eval(self, f, x, x0, evaluation: Evaluation, options: dict):

        # Determine the "jacobian"s
        if (
-            method in ("Newton", "Automatic")
-            and options["System`Jacobian"] is SymbolAutomatic
+            method in ("Newton", "Automatic") and


This looks like a fight between versions of black / autopep8.

In Github Actions, we are using the following versions:
'click==8.0.4' 'black==22.3.0' 'isort==5.10.1'

BlankShrimp force-pushed the solve_ineq branch from a0b44a3 to 00d39ee Compare September 11, 2023 15:28

mmatera reviewed Sep 11, 2023

View reviewed changes

mmatera reviewed Sep 12, 2023

View reviewed changes

BlankShrimp force-pushed the solve_ineq branch from edf05e5 to 6d3d7c7 Compare September 13, 2023 13:06

BlankShrimp requested review from rocky and mmatera September 13, 2023 13:09

BlankShrimp marked this pull request as draft September 13, 2023 13:09

BlankShrimp marked this pull request as ready for review September 13, 2023 13:09

mmatera mentioned this pull request Sep 20, 2023

Moving modules from mathics.algorithm to mathics.eval #922

Merged

BlankShrimp changed the title ~~allow conditions being evaluated in Solve~~ WIP: allow conditions being evaluated in Solve Sep 21, 2023

BlankShrimp force-pushed the solve_ineq branch from 737178d to 8e10644 Compare September 21, 2023 02:36

BlankShrimp changed the title ~~WIP: allow conditions being evaluated in Solve~~ allow conditions being evaluated in Solve Sep 21, 2023

BlankShrimp and others added 5 commits September 21, 2023 10:46

allow conditions being evaluated in Solve

f9a62c4

add test cases

8659983

reconstruct Solve logic

71a7558

alternate old plain solve logic with a recursive one. Now Solve is capable with nested logics

fix test fail issue

405cce1

1. added one missed line and a few catches 2. replace function type hint with 3.9 compatible style

further expand Solve[]'s ability

0ba1a33

1. bring domain check back into solve.eval, so that things like Abs() can be evaluated 2. create system symbols such as System`Reals for domain check 3. refactor, moving most logics out of Solve

BlankShrimp force-pushed the solve_ineq branch from 8e10644 to 0ba1a33 Compare September 21, 2023 02:46

mmatera reviewed Sep 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow conditions being evaluated in Solve #918

allow conditions being evaluated in Solve #918

BlankShrimp commented Sep 10, 2023

mmatera commented Sep 10, 2023 •

edited

BlankShrimp commented Sep 11, 2023

mmatera Sep 11, 2023

mmatera Sep 11, 2023

BlankShrimp Sep 12, 2023

BlankShrimp Sep 12, 2023 •

edited

mmatera Sep 12, 2023

rocky Sep 12, 2023 •

edited

mmatera Sep 11, 2023

BlankShrimp Sep 12, 2023 •

edited

mmatera commented Sep 11, 2023

mmatera Sep 12, 2023

mmatera Sep 12, 2023

rocky commented Sep 14, 2023 •

edited

mmatera commented Sep 17, 2023

BlankShrimp commented Sep 20, 2023

rocky commented Sep 20, 2023 •

edited

mmatera Sep 22, 2023

BlankShrimp Sep 27, 2023

mmatera Sep 27, 2023

mmatera Sep 22, 2023

BlankShrimp Sep 27, 2023 •

edited

mmatera Sep 27, 2023

mmatera Sep 22, 2023

mmatera Sep 22, 2023

mmatera Sep 22, 2023

		return result


		def cut_dimension(evaluation, expressions: Union[Expression, list[Expression]], symbols: Iterable[sympy.Symbol]) -> set[sympy.Symbol]:

allow conditions being evaluated in Solve #918

Are you sure you want to change the base?

allow conditions being evaluated in Solve #918

Conversation

BlankShrimp commented Sep 10, 2023

mmatera commented Sep 10, 2023 • edited

BlankShrimp commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlankShrimp Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlankShrimp Sep 12, 2023 • edited

Choose a reason for hiding this comment

mmatera commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky commented Sep 14, 2023 • edited

mmatera commented Sep 17, 2023

BlankShrimp commented Sep 20, 2023

rocky commented Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlankShrimp Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatera commented Sep 10, 2023 •

edited

BlankShrimp Sep 12, 2023 •

edited

rocky Sep 12, 2023 •

edited

BlankShrimp Sep 12, 2023 •

edited

rocky commented Sep 14, 2023 •

edited

rocky commented Sep 20, 2023 •

edited

BlankShrimp Sep 27, 2023 •

edited