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

Invalid syntax result from Integrate command. #332

Closed
nasser1 opened this issue Jun 3, 2022 · 20 comments
Closed

Invalid syntax result from Integrate command. #332

nasser1 opened this issue Jun 3, 2022 · 20 comments

Comments

@nasser1
Copy link

nasser1 commented Jun 3, 2022

Using 4.0 on Linux, I get the following

mathics

Mathics 4.0.0
on CPython 3.10.4 (main, Apr  2 2022, 09:04:19) [GCC 11.2.0]
using SymPy 1.10.1, mpmath 1.2.1, numpy 1.22.4

Copyright (C) 2011-2021 The Mathics Team.
This program comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions.
See the documentation for the full license.

Quit by evaluating Quit[] or by pressing CONTROL-D.

In[1]:= Integrate[(1 - x^3)^(2/3)/x,x]
Out[1]= --1 ^ (2 / 3) x ^ 2 Gamma[-2 / 3] hyper[{-2 / 3, -2 / 3}, {1 / 3}, 1 / x ^ 3] / (3 Gamma[1 / 3])

Notice the --1 output. This is an error. Mathematica says

PreDecrement::rvalue: 1 is not a variable with a value, so its value cannot be changed.

Btw, as a side issue, what is hyper[ function above? There is no such function in Mathematica.

But the main problem is generating --1

@rocky
Copy link
Member

rocky commented Jun 3, 2022

Btw, as a side issue, what is hyper[ function above?

It probably is SymPy's hyper() and probably appears because Mathics doesn't know of a Mathematica equivalent - perhaps there isn't one.

So this is a translation from SymPy results into the Wolfram Language. In that regard to your other bug mathics/Mathics#1591 which is also related to conversion from SymPy into the Wolfram Language.

Personally, I don't think I will be looking at this for a while. However if you are interested in digging deeper you might check to see what would happen if you wrote this in SymPy and then compare the results.

@DUOLabs333
Copy link

DUOLabs333 commented Jun 5, 2022

This is because Mathics doesn't add parentheses around the leading -1. Here is what sympy returns: -(-1)**(2/3)*x**2*gamma(-2/3)*hyper((-2/3, -2/3), (1/3,), x**(-3))/(3*gamma(1/3)).

@nasser1
Copy link
Author

nasser1 commented Jun 5, 2022

removes the parentheses from the result

Thanks for finding this. Removing parentheses from result is not good thing to do. Hopefully someone familiar with Mathics code could fix this.

@DUOLabs333
Copy link

DUOLabs333 commented Jun 5, 2022

You can get the same thing with -Power[-1,b]. However, -Power[-a,b] is done correctly.

@rocky
Copy link
Member

rocky commented Jun 6, 2022

Thanks for finding this. Removing parentheses from result is not good thing to do. Hopefully someone familiar with Mathics code could fix this.

No one is born being familiar with Mathics code. Mathics is an open source project. That means anyone who is interested enough in solving a problem in the code, can do so.

There are a number of long-standing problems in Mathics that, personally, I want to address before this kind of thing.
When an issue is opened, there is a section for describing why this bug should have priority, or why it would be important for this problem to get solved. Since there was no mention, this like the other issue have low priority.

Basically where one would start to look is in how sympy expressions are converted to Mathics expressions. git grep -n "def from_sympy(" will list the the places where this is done. And that other issue where there was a traceback, that function should also be replaced with a call to from_sympy() as well.

I don't doubt that these bugs will get fixed. But in the scheme of things, unless there is good reason otherwise, for me it is going to be a while before I look at such things.

@DUOLabs333
Copy link

DUOLabs333 commented Jun 6, 2022

Basically where one would start to look is in how sympy expressions are converted to Mathics expressions. git grep -n from_sympy will list the the places where this is done. And that other issue where there was a traceback, that function should also be replaced with a call to from_sympy() as well.

Thanks --- I was looking through the codebase to solve this. Why would that be the culprit --- I was thinking that it had something to do with the implementation of Power, as the bug doesn't occur with negative variables.

@rocky
Copy link
Member

rocky commented Jun 6, 2022

Thanks --- I was looking through the codebase to solve this. Why would that be the culprit --- I was thinking that it had something to do with the implementation of Power, as the bug doesn't occur with negative variables.

Note the corrected grep: git grep -n "def from_sympy(" which shows where the function is defined. The other grep works as well, but it shows all uses which here is too much.

@DUOLabs333
Copy link

DUOLabs333 commented Jun 6, 2022

Where does Mathics determine how to format Power?

@mmatera
Copy link
Contributor

mmatera commented Jun 6, 2022

Something that we could start to do is to create a module to handle hypergeometic functions. You can mimic the code from the exptrig module.

@mmatera
Copy link
Contributor

mmatera commented Jun 6, 2022

Regarding the format of "power" it is defined in the mathics.builtin.arithfns.basic module

@DUOLabs333
Copy link

DUOLabs333 commented Jun 6, 2022

What decides if something is wrapped in parentheses? I see parenthesize in inout.py. Is that it?

@DUOLabs333
Copy link

I go something that works, I just need to deal with the fact that integers don't have precision.

@DUOLabs333
Copy link

Here's my preliminary diff:

diff --git a/mathics/builtin/inout.py b/mathics/builtin/inout.py
index 2d241338..0f740ac4 100644
--- a/mathics/builtin/inout.py
+++ b/mathics/builtin/inout.py
@@ -212,8 +212,15 @@ def parenthesize(precedence, leaf, leaf_boxes, when_equal):
         leaf_prec = leaf.leaves[1].get_int_value()
     else:
         leaf_prec = builtins_precedence.get(leaf.get_head_name())
+    
+    #Give integer precision of Infinity
+    if isinstance(leaf,Integer):
+        leaf_prec=float("inf")
+    
+    #Check if leaf is an integer
+    is_negative = True if isinstance(leaf,Integer) and int(leaf.to_python())<0 else False
     if precedence is not None and leaf_prec is not None:
-        if precedence > leaf_prec or (precedence == leaf_prec and when_equal):
+        if precedence > leaf_prec or (precedence == leaf_prec and when_equal) or is_negative:
             return Expression(
                 SymbolRowBox,
                 Expression(SymbolList, String("("), leaf_boxes, String(")")),

Anything I should change before submitting a PR?

@rocky
Copy link
Member

rocky commented Jun 6, 2022

Does this pass all of the existing tests and doctests? And a test should be added that catches this particular problem.

A quick look and this doesn't feel right. builtins_precendence .get() should be returning the right value so there shouldn't be this test or possibly there is some larger problem.

@DUOLabs333
Copy link

In builtins_precedence, System`Integer isn't defined.

@rocky
Copy link
Member

rocky commented Jun 6, 2022

What I see is a bit different than just a missing parenthesis. The -1 should be used in a Rational[-1, 3]:

$ mathics 

Mathics 4.0.1.dev0
on CPython 3.8.12 (default, Oct 21 2021, 14:26:19) 
using SymPy 1.8, mpmath 1.2.1, numpy 1.21.3, cython 0.29.28

Copyright (C) 2011-2021 The Mathics Team.
This program comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions.
See the documentation for the full license.

Quit by evaluating Quit[] or by pressing CONTROL-D.

In[1]:= Integrate[(1 - x^3)^(2/3)/x,x] // FullForm
Out[1]= Times[Rational[-1, 3], Power[-1, Rational[2, 3]], Power[x, 2], Gamma[Rational[-2, 3]], Power[Gamma[Rational[1, 3]], -1], hyper[List[Rational[-2, 3], Rational[-2, 3]], List[Rational[1, 3]], Power[x, -3]]]

Given that the above shows something a bit different from the OutputForm output, I'd say that the problem is not (just) in from_sympy() but also in a OutputForm formatting routine, possibly as a result of the untranslated symbol.

And the bug with Power is also a bug is the same and not related to from_sympy.

@DUOLabs333
Copy link

What do you mean?

@rocky
Copy link
Member

rocky commented Jun 6, 2022

I mean you may have made a mistake in your sympy translation. Paste the exact sympy session like I did above and the poster of the original issue did.

I thought we went over before the basics of reporting all input given and output shown.

I am sorry, but I do not have the time right now to be able to answer all of your questions.

Also, when I ask questions or ask for something, please answer them, before going off on more questions.

@mmatera
Copy link
Contributor

mmatera commented Jul 26, 2022

Here there is a simpler example:
-1/3*(-1)^(3/4)

The FullForm of this expression is

Times[Rational[-1, 3], Power[-1, Rational[3, 4]]]

but in StandardForm, we get
--1 ^ (3 / 4) / 3

@rocky, I am going to try fixing this before the release.

@mmatera
Copy link
Contributor

mmatera commented Jul 26, 2022

Fixed in #460

@mmatera mmatera closed this as completed Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants