Use pytave for copying variables to python #465

Merged
merged 24 commits into from Aug 14, 2016

Projects

None yet

2 participants

@genuinelucifer
Contributor

python compile function only works on Python 2.
Theoritically, we can use this function for all the type conversion. But here it is used only for @sym objects. Rest of the objects depend on pycall to convert them.

@genuinelucifer
Contributor

I'm not exactly sure that this won't break anything but I couldnt actually find any more use of python_header after copying vars stuff was removed.
And since you yourself commented that calling python_header everytime was sort of waste, I though it would be good to import everything while setting the ipc. Is it ok? Or should I move the library importing stuff into python_ipc_pytave back?

@cbm755 cbm755 commented on an outdated diff Jun 8, 2016
inst/sympref.m
@@ -306,6 +306,9 @@
msg = 'Choosing the default [autodetect] communication mechanism';
case 'pytave'
msg = 'Forcing the PyTave communication mechanism';
+ pyexec(strjoin({'import sympy as sp',
+ 'from sympy import __version__ as spver',
+ 'from sympy import *'}, newl));
@cbm755
cbm755 Jun 8, 2016 Owner

this is not the right place, the first time python_ipc_pytave is called, use a persistent variable to keep track of its the first time.

@cbm755
cbm755 Jun 8, 2016 Owner

anyway, I can't test becuase newl is not defined. Its kinda rude to ask for review when there are trivial problems.

@cbm755
cbm755 Jun 8, 2016 Owner

oh sorry, you didn't ask for review :) ignore the comment about rudeness.

@genuinelucifer
Contributor
genuinelucifer commented Jun 9, 2016 edited

Currently the code is more mature. Although since everything is stored as numpy arrays, we face dificulties in many things. Like:
python_cmd('return 2*_ins[0]', [[2 2], [1 1]]) would work. But,
python_cmd('return 2*_ins[0]', [[2 2]; [1 1]]) won't give required answer...
.
More changes would probably be beneficial on pytave side rather than on symbolic side.
I'll do some changes and probably open a PR onto mtmiller/pytave.

@cbm755
Owner
cbm755 commented Jun 9, 2016

Nice to see this coming along. Few comments:

  1. You need something like:
  if (strcmp(what, 'reset'))
    have_headers = [];
  1. There are lots more imports and workaround that need imported, see python_header.py, probably everything between:
    from sympy.logic.boolalg import Boolean, BooleanFunction
 ---- 8< ----
    from sympy.functions.special.hyper import TupleArg
@genuinelucifer
Contributor

[a,b] = python_cmd('return _ins', 1, 2) works BUT both of
a = python_cmd('return _ins', [1, 2]) and a = python_cmd('return _ins', {1, 2}) give only 2 as result and show warning that number of results do not match.
But, weirdly I get error that second member of output list is not defined if I try:
[a,b] = python_cmd('return _ins', [1, 2]) or [a,b] = python_cmd('return _ins', {1, 2})
.
I have tried many many things but I feel like I am causing even more errors than resolving them...
.
Any suggestions?

@cbm755
Owner
cbm755 commented Jun 11, 2016

I'll take a look tonight. Hang in there, we'll get it!

@cbm755
Owner
cbm755 commented Jun 12, 2016

This doesn't work for me:

>> x = sym(1)
OctSymPy v2.4.1-dev: this is free software without warranty, see source.
Using experimental PyTave communications with SymPy.
  File "<string>", line 35
    _temp = 
           ^
SyntaxError: invalid syntax
error: pycall: 'module' object has no attribute 'pyclear'
error: called from
    python_ipc_pytave at line 110 column 3
    python_ipc_driver at line 59 column 13
    python_cmd at line 166 column 9
    sym at line 365 column 5
    sym at line 236 column 7
>> 
@cbm755 cbm755 and 1 other commented on an outdated diff Jun 12, 2016
inst/private/python_ipc_pytave.m
+ 'from distutils.version import LooseVersion',
+ 'def dictdiff(a, b):',
+ ' """ keys from a that are not in b, used by evalpy() """',
+ ' n = dict()',
+ ' for k in a:',
+ ' if not k in b:',
+ ' n[k] = a[k]',
+ ' return n',
+ 'def Version(v):',
+ ' # short but not quite right: https://github.com/cbm755/octsympy/pull/320',
+ ' return LooseVersion(v.replace(".dev", ""))',
+ '_ins = []',
+ 'def pyclear():',
+ ' global _ins',
+ ' _ins = []',
+ '_temp = '
@cbm755
cbm755 Jun 12, 2016 Owner

wtf is this committed?

@genuinelucifer
genuinelucifer via email Jun 12, 2016 Contributor
and others added some commits Jun 12, 2016
@cbm755 WIP: rewrite recursion when storying variables
The function store_vars_in_python processes a lists (cell-arrays).
It calls itself if it hits another cell array.  This is just a first
draft, needs some fixes...
77d2b2f
@genuinelucifer genuinelucifer Fix typo and use correct function to store sym obj 7cfd4b7
@cbm755 Merge branch 'pytave_ex_recurse' into HEAD e702622
@cbm755 fix-up sym importing
uses `@pyobject`: there might be a more direct way.
b61fda9
@cbm755
Owner
cbm755 commented Jun 12, 2016

Here's a command I used for testing:

>> [a,b] = python_cmd('print(_ins); return [1,2]', "one", x, y, "two", {"hello", 30}, 40)
['one', x, y, 'two', ['hello', 30.0], 40.0]
a =  1
b =  2

(note we can print in python for debugging)

@cbm755
Owner
cbm755 commented Jun 12, 2016

Another useful thing for testing:

>> pycall('pystoretemp', 42)
>> pyexec('print _temp')
[[ 42.]]
@cbm755
Owner
cbm755 commented Jun 12, 2016

Does this have the #456 fix? I looks like it does but it doesn't work:

>> L = python_cmd('return (1,2),')
warning: number of outputs don't match, was this intentional?
warning: called from
    python_cmd at line 188 column 5
L =  1
>> 
@genuinelucifer
Contributor

It should have that fix. I cherry picked from here only. Probably I messed
something else too. I'll try to fix it.
On Jun 12, 2016 10:50 PM, "Colin Macdonald" notifications@github.com
wrote:

Does this have the #456 #456
fix? I looks like it does but it doesn't work:

L = python_cmd('return (1,2),')
warning: number of outputs don't match, was this intentional?
warning: called from
python_cmd at line 188 column 5
L = 1


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#465 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AK38VY03zGWrj6ao4eK3uQ7OW5D5n8tzks5qLD_QgaJpZM4Iw0tL
.

@genuinelucifer genuinelucifer and 1 other commented on an outdated diff Jun 19, 2016
inst/private/python_ipc_pytave.m
+ 'from sympy import *',
+ 'from sympy.logic.boolalg import Boolean, BooleanFunction',
+ 'from sympy.core.relational import Relational',
+ '# temporary? for piecewise support',
+ 'from sympy.functions.elementary.piecewise import ExprCondPair',
+ 'from sympy.integrals.risch import NonElementaryIntegral',
+ 'from sympy.matrices.expressions.matexpr import MatrixElement',
+ '# for hypergeometric',
+ 'from sympy.functions.special.hyper import TupleArg',
+ 'from sympy.utilities.iterables import uniq',
+ 'import copy',
+ 'import binascii',
+ 'import struct',
+ 'import codecs',
+ 'import xml.etree.ElementTree as ET',
+ 'from distutils.version import LooseVersion',
@genuinelucifer
genuinelucifer Jun 19, 2016 Contributor

are all these imports required? I merely copy pasted all of them because some tests showed error regarding a few of the imports..

@cbm755
cbm755 Jun 20, 2016 Owner

You should be able to drop xml, binascii.

Probably/maybe codecs, copy, struct too.

The SymPy stuff is probably all needed: this is because these things appear in srepr for some SymPy object, so we need them to copy objects back to SymPy. Worse, they need to be in the main namespace... Possibly git blame inst/private/python_header.py will tell you more about each of them.

@genuinelucifer
genuinelucifer Jun 20, 2016 Contributor

Thanks. I did git blame but it only shows which changes in this file were made when/by whome.
But, some imports, although not required by python_header.py,are still required by some tests...
Since the imports are not that many, I'll try to remove them one by one and try.

@cbm755
cbm755 Jun 20, 2016 Owner

Just FYI, let me walk you through it:

git blame python_header.py

---- 8< ----
7b6c35b5 (Colin B. Macdonald 2014-11-16 15:23:32 +0000  35)     from sympy.functions.elementary.piecewise import ExprCondPair
51a80384 (Colin B. Macdonald 2016-01-07 00:26:37 -0800  36)     from sympy.integrals.risch import NonElementaryIntegral
9474bd5a (Colin B. Macdonald 2016-01-11 00:05:02 -0800  37)     from sympy.matrices.expressions.matexpr import MatrixElement
400811d8 (Colin B. Macdonald 2016-05-09 13:04:12 -0700  38)     # for hypergeometric
400811d8 (Colin B. Macdonald 2016-05-09 13:04:12 -0700  39)     from sympy.functions.special.hyper import TupleArg
af6e79e7 (latot              2016-01-17 02:01:21 -0300  40)     from sympy.utilities.iterables import uniq
---- 8< ----

So lets look at this one: from sympy.integrals.risch import NonElementaryIntegral:

git show 51a80384

commit 51a80384d5caea0db211e452d39f0b4f6b3778cc
Author: Colin B. Macdonald <cbm@m.fsf.org>
Date:   Thu Jan 7 00:26:37 2016 -0800

    Fix passing NonElementaryIntegrals back to Python

    Fixes https://savannah.gnu.org/bugs/index.php?46831.

    Yet another case where `eval(srepr(...))` is not quiet the
    identity map.

(this doesn't always work b/c the most recent change to a line might not explain why that line is there...)

@cbm755
cbm755 Jun 20, 2016 Owner

hmmm "not quite the identity map": someone is not proofreading his commits :-)

@cbm755
cbm755 Jun 23, 2016 Owner

You should be able to drop xml, binascii.

ping

@genuinelucifer
genuinelucifer Jun 23, 2016 Contributor

sorry, never got around to test it. I'll do it now.

@cbm755
Owner
cbm755 commented Jun 20, 2016

I've tried this again: it seems much worse than the pytave branch right now, lots of things are failing on return from SymPy. But this branch is about copying into Python...? What is happening?

findsymbols failing for example...

@genuinelucifer
Contributor

lots of things are failing on return from SymPy. But this branch is about copying into Python...? What is happening?

Hmm... Sorry I forgot to deal with this branch in the past week. I'll try to see today what exactly is going wrong. And yes, now that you mention it, since it is just copying into python the return statements should not cause octave to crash (I am getting octave crash on many tests).

@genuinelucifer
Contributor

@cbm755 This feels like a lucky day! ^_^
Even this branch is looking good now. I got no crashes and only 25 tests failed! :)
Looks like we will be merging this branch soon.

@genuinelucifer
Contributor

This is really going great! :D
Now, we have ONLY 9 failing tests!!
.
3 tests fail in @sym/solve.m which will be fixed when #486 is done.
6 tests fail in python_cmd.m which are due to escaping characters in strings..
.
I can fix the tests in @sym/solve.m but I have no clue how to fix tests in python_cmd.m...
Ex (One of the tests that fails):

>> x = 'a string\nbroke off\nmy guitar\n';
>> x2 = sprintf(x);
>> y = python_cmd ('return _ins', x);
a string\nbroke off\nmy guitar\n
>> x3 = strrep(x2, sprintf('\n'), sprintf('\r\n'));  % windows
>> assert (strcmp(y, x2) || strcmp(y, x3))
error: assert (strcmp (y, x2) || strcmp (y, x3)) failed
error: called from
    assert at line 92 column 11
>> x2
x2 = a string
broke off
my guitar

>> x3
x3 = a string
broke off
my guitar

>> y
y = a string\nbroke off\nmy guitar\n

Any pointers?

@cbm755
Owner
cbm755 commented Jun 21, 2016

I'll look into python_cmd: there is at least one weird test in there...

@cbm755
Owner
cbm755 commented Jun 21, 2016

I filed https://bitbucket.org/mtmiller/pytave/issues/24/pyexec-inconsistent-error-handling about some of the error handling stuff.

re: the string failures: feels like we are doing the right thing: those tests look more like enforcing the limitations of the other IPC mechanisms. I'll deal with that.

@genuinelucifer
Contributor
@cbm755
Owner
cbm755 commented Jun 21, 2016

pull from pytave to fix the string stuff

@genuinelucifer
Contributor

Now, only 2 tests fail in python_cmd that too they are expected to fail. But they expect an error message while they get a python exception.
Probably lowest priority for now. #457 is the cause.

@cbm755
Owner
cbm755 commented Jun 22, 2016

For the "very specialized test", I suggest making it %!xtest error <AttributeError> and adding a comment to the pytave bug.

For the "export type" one, that probably should work with @pyobject. Could just xtest for now with a note that it will succeed on pytave.

@genuinelucifer
Contributor

We now have only 2 tests failing. Changing them to xtest will fail the other IPCs that's why I didn't change them.... Should I?

@cbm755
Owner
cbm755 commented Jun 22, 2016

either passing or failing an xtest is ok (this differs from doctest XFAIL which must fail).

@cbm755
Owner
cbm755 commented Jun 22, 2016

This PR is getting a bit unwieldy. Can you submit the xtest change we just talked about to pytave branch as a new PR? I think that is unrelated to this PR. Same for any other changes related to python-to-octave (dict stuff?).

@genuinelucifer
Contributor

I removed the 2 commits from this branch. And submitted 2 PRs...
.
Also, Should I merge commits on this PR?
Because I guess we have covered all the cases (that are currently being used)...

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/store_vars_in_python.m
+ persistent counter = round(100000*rand())
+
+ for i = 1:numel(L)
+ %i
+ x = L{i};
+ if (isa(x, 'sym'));
+ pyexec([varname '.append(' char(x) ')'])
+ elseif (iscell (x))
+ tempname = [varname num2str(counter)];
+ counter = counter + 1;
+ pyexec ([tempname ' = []'])
+ store_vars_in_python (tempname, x)
+ pyexec([varname '.append(' tempname ')'])
+ elseif (isscalar (x) && isnumeric (x))
+ % workaround upstream PyTave bug: https://fixme.url.here
+ % FIXME: add bug number, about making arrays [[42]] when passing 42
@cbm755
cbm755 Jun 23, 2016 Owner

please do these fixme's

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/store_vars_in_python.m
@@ -0,0 +1,31 @@
+function store_vars_in_python (varname, L)
+%varname
+%L
@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/store_vars_in_python.m
@@ -0,0 +1,31 @@
+function store_vars_in_python (varname, L)
+%varname
+%L
+ persistent counter = round(100000*rand())
@cbm755
cbm755 Jun 23, 2016 Owner

I noted this before somewhere but its still here: why we have this rand number? Can just set to 0?

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/store_vars_in_python.m
+ %i
+ x = L{i};
+ if (isa(x, 'sym'));
+ pyexec([varname '.append(' char(x) ')'])
+ elseif (iscell (x))
+ tempname = [varname num2str(counter)];
+ counter = counter + 1;
+ pyexec ([tempname ' = []'])
+ store_vars_in_python (tempname, x)
+ pyexec([varname '.append(' tempname ')'])
+ elseif (isscalar (x) && isnumeric (x))
+ % workaround upstream PyTave bug: https://fixme.url.here
+ % FIXME: add bug number, about making arrays [[42]] when passing 42
+ pycall ('pystoretemp', x)
+ if isinteger(x)
+ pyexec ([varname '.append(int(_temp[0,0]))'])
@cbm755
cbm755 Jun 23, 2016 Owner

Can you add something like:

% FIXME: is this working around a PyTave bug?  without it type is e.g., numpy.uint8
@cbm755 cbm755 commented on the diff Jun 23, 2016
inst/private/store_vars_in_python.m
@@ -0,0 +1,31 @@
+function store_vars_in_python (varname, L)
@cbm755
cbm755 Jun 23, 2016 Owner

Can you put a GNU copyright header on this?

@cbm755
Owner
cbm755 commented Jun 23, 2016

Also, Should I merge commits on this PR?

You mean squash I guess. Do you think it needs it? It looks ok to me. Are there any binary files or terribly embarrasing code in there? ;-) If not, its fine as is for me.

@genuinelucifer
Contributor

You mean squash I guess.

Oops, yes... Seems ok to me too. won't squash..

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/store_vars_in_python.m
+
+ for i = 1:numel(L)
+ x = L{i};
+ if (isa(x, 'sym'));
+ pyexec([varname '.append(' char(x) ')'])
+ elseif (iscell (x))
+ tempname = [varname num2str(counter)];
+ counter = counter + 1;
+ pyexec ([tempname ' = []'])
+ store_vars_in_python (tempname, x)
+ pyexec([varname '.append(' tempname ')'])
+ elseif (isscalar (x) && isnumeric (x))
+ % workaround upstream PyTave bug: https://bitbucket.org/mtmiller/pytave/issues/14
+ pycall ('pystoretemp', x)
+ if isinteger(x)
+ % workaround as PyTave apparently stores everything as Float
@cbm755
cbm755 Jun 23, 2016 Owner

no, its more complicated then that. It goes to numpy.uint8 or someother numpy type. your int here brings it to native python int type. please leave an explicit "FIXME" until we decide if that is pytave problem or not.

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/python_ipc_pytave.m
+ 'import copy',
+ 'import binascii',
+ 'import struct',
+ 'import codecs',
+ 'import xml.etree.ElementTree as ET',
+ 'from distutils.version import LooseVersion',
+ 'def dictdiff(a, b):',
+ ' """ keys from a that are not in b, used by evalpy() """',
+ ' n = dict()',
+ ' for k in a:',
+ ' if not k in b:',
+ ' n[k] = a[k]',
+ ' return n',
+ 'def Version(v):',
+ ' # short but not quite right: https://github.com/cbm755/octsympy/pull/320',
+ ' return LooseVersion(v.replace(".dev", ""))',
@cbm755
cbm755 Jun 23, 2016 Owner

can we remove the commas from the end of these lines? some have some don't, I guess I prefer none.

@cbm755 cbm755 commented on an outdated diff Jun 23, 2016
inst/private/python_ipc_pytave.m
s = strjoin(cmd, newl);
- pyexec(s)
-
+ pyexec(s);
@cbm755
cbm755 Jun 23, 2016 Owner

unnecessary change

@genuinelucifer
Contributor

@cbm755 Done. I hope didn't miss anything..

@cbm755
Owner
cbm755 commented Jun 23, 2016

Cool, I'll plan to merge this when tests pass (nice to be able to say that again!)

(If the tests fail, try pulling from pytave.)

@genuinelucifer
Contributor

Well, 3 tests will fail... But, after merging to pytave they won't fail. Due to the dicts fix...
.
Should I merge pytave here?

@cbm755
Owner
cbm755 commented Jun 23, 2016

Yeah merge here, it keeps segfaulting. (I wasn't sure if travis runs this branch or the result of this branch, merged to the target).

@genuinelucifer
Contributor

I saw the segfaults now. On my pc (without merge), the tests were running fine (with 3 failing tests)..
Donno what happened on travis...

@genuinelucifer
Contributor
genuinelucifer commented Jun 23, 2016 edited

@cbm755 Any pointers here? I have no clue what is going wrong. None of the changes seem too drastic to cause a segfault!

@cbm755
Owner
cbm755 commented Jun 23, 2016

Possibly this branch depends on pyobect somehow?

@cbm755
Owner
cbm755 commented Aug 9, 2016

What's the status of this? Pytave does a lot more now, it would be good to update this if necessary. I'd like to merge this to my pytave branch (and hopefully soon to master).

(I should think about #506 but wanted to check if this is up-to-date first)

@genuinelucifer
Contributor

Well, for me most of the case pass. I don't think we'll need changes here. Probably we can drop a few things.

And as you said travis only has 4.0.x so we can't test there. I'll try to update this (if needed) by evening (~5-6hrs)..

@cbm755
Owner
cbm755 commented Aug 9, 2016

Travis support is not urgent. Please test this branch against pytave master and let me know if you think we can start to merge things.

@genuinelucifer
Contributor

I have done some more testing and it seems that we can safely merge this to pytave after #507 is resolved upstream...

@cbm755
Owner
cbm755 commented Aug 13, 2016

Can you update check_and_convert and the to use the newest Pytave features? Also, add a copy copyright header?

Re: dict: maybe for now, we should convert them to structs.

@genuinelucifer
Contributor

@cbm755 I will send a new PR for the changes in check_and_convert as PR was only for store_vars_in_python...

Please review this and merge if it's ok.. I'll send the other PR with the dict change in a couple of hours.

@cbm755
Owner
cbm755 commented Aug 14, 2016

Ok, I'll merge this to the pytave branch. I think the import can be re-written with newer pytave features but we can do it in a new PR.

@cbm755 cbm755 merged commit badf3af into cbm755:pytave Aug 14, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@genuinelucifer genuinelucifer deleted the genuinelucifer:pytave_ex branch Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment