1. Mike Miller
  2. pytave
  3. Pull requests

Pull requests

#31 Merged at d8e2443
Repository
Deleted repository
Branch
default (f2b4576879a6)
Repository
pytave
Branch
default

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

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...