Drop conversion of lists, dicts and tuples (fixes issues #27, #26)

#25 Merged at 6e83efb
Deleted repository
default (9278a272b1c8)
  1. Abhinav Tripathi
  • python_to_octave.cc: Remove the code that attempts to convert lists, dicts and tuples from python to octave cell arrays. Instead convert them to pyobjects.
  • pycall.cc, pyeval.cc: Do not attempt conversion to pyobject and rather issue proper error message for the exception. Change tests to consider this change.

Comments (17)

  1. Colin Macdonald

    What a pleasant amount of code removal ;-)

    Did we decide what to do about numpy arrays of doubles? Maybe those should still be converted? @Mike Miller what did you intend? (ideally, we would support something like COW for double arrrays---later!)

  2. Mike Miller repo owner

    I know we can always bring back deleted code later, but for now let's try to be minimal about this change and only remove the logic that causes lists, dicts, and tuples to be converted automatically here.

    I think keeping auto-conversion of Octave arrays into numpy arrays and back again is good to preserve, and we might still want the routines that convert to other types in the future.

  3. Abhinav Tripathi author

    Strangely enough if I leave the array conversion then lists are not being properly converted to pyobject:

    >> pyeval ("[40, 40]")
    ans = [pyobject 0x7f9244fbd128]
      [40, 40]
    >> pyexec ("def pyfunc(x):\n    return 2*x");
    >> pycall ("pyfunc", [20 20])
    ans =
       40   40
    1. Abhinav Tripathi author

      @Mike Miller This is probably because octave_to_python converts it to numpy array and then python_to_octave converts it to octave list and not a pyobject. Should I remove the array conversion or let it be like this for now?

    2. Colin Macdonald

      (I replied earlier from my phone but don't see it here: do email replies to bitbucket work?)

      Anyway, this example looks correct to me. I don't see a problem.

      First return a python list. 2nd passes a double array to numpy array and then returns a double array: looks good.

      Note: the [20 20] thing is not a list! Its a double array.

      1. Abhinav Tripathi author

        this example looks correct to me. I don't see a problem.

        Oh. I guess I took it that we are converting every list-type-of class to pyobject...

        Note: the [20 20] thing is not a list! Its a double array.

        Well, now I am confused. I thought in octave there are matrices and we have been treating 1 D matrix as a list!?

        1. Colin Macdonald

          This is a cell array:

          {1 2.3 "hello" pi}

          It becomes a list in Python. It comes back to Octave as a pyobject wrapping a list.

          This is a double array:

          [1 2.3 4.5]

          It becomes a numpy array. A numpy array should come back to Octave as a double array.

          So in summary---at least for our purposes---a 1D matrix is not a list.

  4. Abhinav Tripathi author

    Sorry if there might have been noise (mails) as I forgot some minor changes in the PR a couple of times. I have now updated it to remove conversion of only lists, dicts and tuples to octave cell arrays.

  5. Mike Miller repo owner

    Ok, there's a slight merge conflict now but I can take care of that on my end, and I'll run through all the tests and see where we are. I may add some tests to pycall as well.

  6. Mike Miller repo owner

    @Abhinav Tripathi I merged this, but you did miss a little bit with this change.

    • The code that you removed in pyeval.cc has equivalent code in pycall.cc that you didn't remove (builtins_module, calculating the id, all now unused code).
    • Some equivalent tests could be added to pycall to do things like pycall ("list"), etc.

    Also there are now a handful of failing tests in @pybobject methods due to this change. This is not a bad thing, these are things that should break, but you should get in the habit of running all of the tests when you make changes, and either fix tests that now break or point out what other changes will be necessary after this is merged. In this case it is mostly fieldnames and methods needing to adapt to the new return value convention.

    1. Mike Miller repo owner

      So please file another PR that takes care of the housekeeping of removing the dead code from pycall.cc, and add some unit tests to pycall for list, dict, and tuple. These can be one commit or two separate commits in a single PR, either way you want to do it.

  7. Colin Macdonald

    Mike, sorry for giving "+1" without properly testing. It might be helpful if you shared your work flow for running tests. I suppose its something like:

    __run_test_suite__ ({"."}, {})