sym() and vpa() refactor #804

cbm755 · 2017-05-18T22:14:13Z

I feel like I could tweak this forever, so here's some progress anyway. I've taken apart from of @latot's work to centralize const_to_python_str (which dealt with doubles and strings). That logic is a little more spread out now, but IMHO its clearly to understand @sym/sym which is the code that matters.

There were also sublte differences between what should happen with e.g., pi in sym(pi, 'f'), sym(pi, 'r') and vpa(pi).

This also fixes a bug I introduces where sym(inf, 'f') was zero.`

Finally, it fixes #604 by allowing sym('i') to make a variable named i rather than imaginary unit, which is for SMT compatibility.

The double input part of the code no longer uses "const_to_python_str".

Fixes #604.

It does more than we need: we were only using it for the double conversion. Even there, it converted doubles-that-are-integers to strings which seems unncessary here.

It doesn't need to deal with doubles anymore

Fixes #799. This is for compatibility with SMT. I've slightly changed the heuristic: we return whichever choice minimizes the error.

This is not supposed to change anything

Remove the "check" flag by more agressively calling python_cmd as soon as we know its arguments instead of more complicated logic which tried to defer calling python_cmd.

Instead of using a flag, return empty or the new string. Document the private fcn a bit.

cbm755 · 2017-05-18T22:23:51Z

I guess this is mostly @latot's stuff: what do you think? Its probably hard to review overall but if you go commit-by-commit, I hope each one is sensible.

[skip-ci]

latot · 2017-05-19T14:22:12Z

inst/@sym/private/double_to_sym_heuristic.m

+    y = python_cmd ('return -S.Pi');
+  elseif ((abs (x) < flintmax) && (mod (x, 1) == 0))
+    y = python_cmd ('return Integer(_ins[0])', int64 (x));
+  else


i would like keep here some loop instead of check every var line by line....

i think is more easy to handle, or maybe with less code?

I figured you might say this ;-) I respect that point of view, but I disagree. To me a bunch if elseif is readable and clear. Ultimately just a matter of style.

But if that list gets much longer in the future, then maybe I prefer your approach.

latot · 2017-05-19T14:28:53Z

inst/@sym/sym.m

@@ -277,7 +277,6 @@
  end

  asm = {};
-  check = true;


I was trying to avoid the most possible call python_cmd every time, the main reason is #561, to can use classdef we will run an extra lines script every time we call that function for the limitations, so we can avoid write a lot of code and reduce it....

If you want remove it maybe instead keep it as comments? Just to don't loose the idea.

I'm not sure I understand. Is this about sym explicitly calling itself? I thought we were not supposed to have the sym ctor explicitly calling sym(...), but it was ok if sym called some other function which then called sym(...).

What do you mean by "extra lines script every time we call that function for the limitations"? Can you give me an explicit bit of code that will need to change?

Sorry, but I've kind of lost track of the various classdef issues we face.

Does this comment need to be addressed before I merge this?

Would adding this comment address your concern?

% Note: a previous version of this code deferred most calls to `python_cmd % until near the end of the function. Now, it is called in many places. Its % possible this has some negative consequences for a future port to classdef. % If you're reading this in the future and have successfully done that port, % then please delete this comment ;-)

Otherwise, what should it say? (I don't understand what the issue is here so harder for me to make a precise statement).

if i remember right this was the reason https://savannah.gnu.org/bugs/?49421, in classdef we can't set properties, so every time when we try to set it we need to run the "Copy" function, other way maybe is store the result in a new var and set it in the end of the script, then we can call the function only one time.

Ok, I think I see, thank you.

There are paths in #590 that end early with s = meh...; return, so I don't see this PR's code as being much worse. I will merge for now, fight with classdef when we get there ;-)

latot · 2017-05-19T14:35:46Z

inst/vpa.m

-  elseif (ischar(x))
-    x = magic_str_str(x);
+  elseif (ischar (x))
+    if (strcmp (x, 'inf') || strcmp (x, 'Inf') || strcmp (x, '+inf') || ...


Hi, i maybe instead of this vpa we can simplify to:

function r = vpa(x, n) if (nargin == 1) n = digits(); elseif (nargin ~= 2) print_usage (); end n = sym(n); cmd = { 'x, n = _ins' 'return sympy.N(x, n),' }; r = python_cmd (cmd, x, n); end

Actually pass all the tests, and seems works.

All the check we do here are basically what sym function does.

Wow, cool, I will think about this! Did you mean x = sym(x)? Otherwise I don't see how that could work.

i do something really bad when i test this, is necessary check this.

latot · 2017-05-19T14:39:06Z

inst/@sym/private/double_to_sym_heuristic.m

+      end
+    else
+      y = python_cmd ('return sqrt(Integer(*_ins))', int64 (N3));
+    end


Thinking in the future, this is a great idea, to complement it i would like.. "split"? the differents methods to get the aproximation, so then we can get this file to be more easy to edit, i'm thinking to can add some "functions" and then the script run all in some loop and choose the best.

I suppose that could be done in the future. Although I'm not sure there will be more heuristics: I read through the SMT docs and it sounds like these are the three cases they do.

latot · 2017-05-19T14:41:19Z

inst/@sym/private/double_to_sym_exact.m

  else
-    s = x;
+    %% Rational will exactly convert from a float
+    y = python_cmd ('return Rational(_ins[0])', x);
  end


same as below

#804 (comment)

latot · 2017-05-19T14:44:16Z

inst/@sym/sym.m

+    iscmplx = ~isreal (x);
+    if (iscmplx && isequal (x, 1i))
+      s = python_cmd ('return S.ImaginaryUnit');
+      return


this part is related to the "check" comment.

latot · 2017-05-19T14:48:51Z

Ready!, there is greats changes, there is the comments.

cbm755 added 10 commits May 18, 2017 15:09

@sym/sym: fix ratflag 'f' inf/nan by refactoring

267f7fe

The double input part of the code no longer uses "const_to_python_str".

sym and vpa: don't make imaginary units from string 'i' or 'I'

58e05b9

Fixes #604.

vpa: drop const_to_python_str usage

8e7e40f

It does more than we need: we were only using it for the double conversion. Even there, it converted doubles-that-are-integers to strings which seems unncessary here.

refactor private function

46153d9

It doesn't need to deal with doubles anymore

@sym/sym: heuristics should look for sqrt(integer) as well

2fdda3c

Fixes #799. This is for compatibility with SMT. I've slightly changed the heuristic: we return whichever choice minimizes the error.

rename private function

c55801e

@sym/sym: refactor out the double-to-sym code to some helper fcns

5489ff5

@sym/sym: minor tweak to integer-to-sym conversion

9b02752

This is not supposed to change anything

@sym/sym: refactor, simplifying logic by removing a flag

81dea89

Remove the "check" flag by more agressively calling python_cmd as soon as we know its arguments instead of more complicated logic which tried to defer calling python_cmd.

@sym/sym: private function refactor

4bc64da

Instead of using a flag, return empty or the new string. Document the private fcn a bit.

@sym/sym: update docs for sqrt(integer) heuristic

4b02016

[skip-ci]

latot reviewed May 19, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into sym_vpa_refactor

8b11cc5

cbm755 merged commit 6390152 into master Jul 24, 2017

cbm755 deleted the sym_vpa_refactor branch July 24, 2017 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sym() and vpa() refactor #804

sym() and vpa() refactor #804

cbm755 commented May 18, 2017

cbm755 commented May 18, 2017

latot May 19, 2017

latot May 19, 2017

cbm755 May 19, 2017

latot May 19, 2017

cbm755 May 19, 2017

cbm755 May 19, 2017

latot May 20, 2017

cbm755 Jul 24, 2017

latot May 19, 2017

cbm755 May 19, 2017

latot May 21, 2017

latot May 19, 2017

cbm755 May 19, 2017

latot May 19, 2017

latot May 19, 2017

latot May 19, 2017

latot commented May 19, 2017

sym() and vpa() refactor #804

sym() and vpa() refactor #804

Conversation

cbm755 commented May 18, 2017

cbm755 commented May 18, 2017

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

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

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

Choose a reason for hiding this comment

latot commented May 19, 2017