Edit test and functions to adapt to lists, dicts being pyobjects

#31 Merged at d8e2443
Repository
Deleted repository
Branch
default (f2b4576879a6)
Repository
pytave
Branch
default
Author
  1. Abhinav Tripathi
Reviewers
Description

Fix cell indexing for pyobject in subsref

  • @pyobject/subsref.m: Allow indexing pyobjects with cell type indexing with ranges and {:}

Use new subsref indexing to fix the failing tests

  • @pyobject/methods.m, @pyobject/fieldnames.m: Use the new subsref indexing to convert the pyobject to octave cell before returning

Edit tests in subsref to use and verify cell indexing

  • @pyobject/subsref.m: Edit an existing test to use the available cell indexing syntax and also add a test to verify that None is returned if nargout > 0

Comments (16)

  1. Abhinav Tripathi author

    To save myself from creating a new function to convert pyobject to octave cell, I took the liberty of editing subsref to support range indexing like s{1:3} which was not supported currently. I hope it is fine.
    This PR is mostly meant to cover what was left in the PR #25 which dropped the conversion of lists, dicts and tuples.
    Please review @Colin Macdonald @Mike Miller

  2. Mike Miller repo owner

    No, sorry, I don't think this needs more work.

    See https://www.gnu.org/software/octave/doc/interpreter/Indexing-Cell-Arrays.html for how cell arrays are indexed in Octave. Python object indexing should work the same way, see https://www.mathworks.com/help/matlab/data-types_buik_wp-4.html.

    x = py.list (...)
    x{1}    ## should return x[0]
    x{1:5}  ## should return a cs-list of elements, nargout = 5
    x(1)    ## should return a py.list equivalent to x[0:1]
    x(1:5)  ## should return a py.list equivalent to x[0:5]
    

    These indexing types are not yet implemented in subsref, except for the degenerate case of x{1} which returns a single object.

    I think what we need to solve fieldnames and methods is a way to convert a list into a cell array:

    y = cell (x)
    

    which could be implemented as

    c = {x{:}};
    

    So if you can fix subsref to return multiple output values instead of a cell array, that will be Matlab compatible and better.

    The doc fix in dummy.m and the clean up and tests in pycall.cc should each be separate commits (maybe a separate PR also).

    1. Abhinav Tripathi author

      So if you can fix subsref to return multiple output values instead of a cell array, that will be Matlab compatible and better.

      I tried many things but could not get the desired result.
      To clarify, this is what cell array indexing with a range looks like on my octave:

      >> s = {1, 2, 3, 4, 5, 6};
      >> s{1:3}
      ans =  1
      ans =  2
      ans =  3
      

      The confusing thing for me are the 3 'ans' values!!

      The best I could get was this (I made a new function to test so that posting the code here as comment would be feasible...

      >> function  varargout = myfun()
      
        r = [1:10];
      
        for i=1:10
      
         varargout{i} = r(i);
      
        endfor
      
      endfunction
      
      >> myfun()
      ans =  1
      >> [a, b] = myfun()
      a =  1
      b =  2
      >> [a, b, c] = myfun()
      a =  1
      b =  2
      c =  3
      

      This way I can get as many arguments I want as a cs list. But the first case of just myfun(), I get only 1 ans!

      After searching a lot, I found out that there exists something call vr_val to return multiple values. But apparently it does NOT exist in my build of octave. Help please...

      1. Mike Miller repo owner

        This is working correctly. When a normal function returns 5 return values, but you call it with zero or one lvalue, Octave silently truncates the remaining return values.

        myfun
        a = myfun
        [a, b] = myfun
        

        myfun is called with nargout set to 0, 1, and 2 respectively.

        But for the special case of subsref, if Octave sees that it looks like cell indexing {}, it will call the subsref function with nargout set to the number of indices.

        x{1}
        x{1:2}
        x{1:10}
        

        subsref is called with nargout set to 1, 2, and 10 respectively.

        If you make a class @foo and change myfun to subsref, you should see this working the same as cell array indexing

        function varargout = subsref ()
          varargout = num2cell (1:nargout);
        endfunction
        
  3. Abhinav Tripathi author

    Ok, I have pushed my changes here. But, still it doesn't work as expected...
    Specifically, if I do x{:} or x{1:3} with a pyobject, only 1 ans is returned and hence I cannot convert it to a cell.
    While, if I do the same with a cell array then x{:} will return all the elements as separate ans values and x{1:3} will give 3 ans values and hence both can be wrapped in a cell array like {x{:}} or {x{1:3}}...
    Also, if I do [a,b] = x{1:3} with a cell array then 1st 2 values will be assigned without any warning that's why I didn't assert on nargout...

    I will push the changes to pycall.cc and dummy.m as a separate PR in sometime.

  4. Mike Miller repo owner

    Thank you for updating this. I've tested a few things and come to the same conclusion. I am going to report an Octave bug. I think I can get this fixed on Octave's default branch by the next release. Currently subsref works differently depending on whether it's an old-style struct-based class or a classdef class. We'll have to work around this bug for now. For example we'd like to simply do

    val = {x{:}};
    

    but instead we'll have to do

    [val{1:len}] = x{:};
    

    where we have calculated how many elements should be in val. Or instead of what you have in fieldnames.m, instead something like

    [names{1:len}] = subsref (...
    

    Calling x{...} directly will still not work correctly until we fix the bug in Octave.

    Good catch!

  5. Abhinav Tripathi author

    Although all other things were fixed now. But 2 tests started failing now. The thing is, if we do z = pyeval("[1, [21, 22]]") then z{2}{1} is giving errors. While s = z{2}; s{1} is working perfectly! I am trying many things hopefully I'll arrive at a solution and update this PR soon.

    1. Abhinav Tripathi author

      Okay most of the problems have been solved now. I will push the changes. Only 1 problem remains. How do I differentiate if the user called z{:} or z{":"}? Both are giving the same result which is causing problem when : is a key in a dict (it is in a test currently).

      1. Mike Miller repo owner

        You can't, they are the same thing, good catch.

        When the Python object is a Mapping object, then it will be interpreted as a literal key with the name :. When the Python object is a Sequence, then it will be interpreted as meaning the entire sequence. Does that work?

        1. Abhinav Tripathi author

          Oh, thanks. So, now I have implemented it such that : in a dict only searches for the key. While in other sequences it returns every element. I will rebase this PR and update it now it has some minor merge conflicts.

          1. Mike Miller repo owner

            I would recommend one commit to add the improved indexing to subsref, and a separate commit to adapt fieldnames and methods to extract the lists of names using the new subsref capabilities.

            1. Abhinav Tripathi author

              Sure. I am currently building the update pytave and running tests. Also adding missing spaces at a few places. I will add 2 commits to the PR once that is done.

  6. Abhinav Tripathi author

    @Colin Macdonald Thanks. After removing all the "{}" specific return code too it is working perfectly. I put it there in the start as I was returning a cell array but since that changed I forgot to try it without that. Also I added the new test that you said. I have fixed all the spacing problems and incoporated your other comments as well.

    I have split the changes into 3 commits now. I hope it's okay. Please review again @Colin Macdonald @Mike Miller

    1. Mike Miller repo owner

      I see no reason not to merge this now. Colin?

      There are a couple of minor bugs I've hit while testing this, but easy changes I can apply on top of these after merging.

      1. Colin Macdonald

        OK. I also see some changes.

        Things that need fixed, could be done with new PRs: for example, the test for ":" should be inside the isscalar() test.

        Also, methods and fieldnames should use {:} instead of {1:length}. I don't really understand why 1:length works at all yet to be honest. But anyway, we will soon replace those bits with calls to cell...