bugGNU Octave - Bugs: bug #51586, Creating method handle fails using...

 
 

bug #51586: Creating method handle fails using str2func

Submitter:  Piotr Held <jsoh425>
Submitted:  Thu 27 Jul 2017 12:07:47 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  None Assigned to:  None
Originator Name:  Piotr Held Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 30 Aug 2017 06:02:35 PM UTC, comment #12: 

So does that mean lazy lookup of function handles is something acceptable in Octave? I thought that it was decided in Bug #31821 that we shouldn't have lazy lookup.

If that's the case then I'll stop working on my patch to do more complex function handle assignments at creation.

Piotr Held <jsoh425>
Tue 29 Aug 2017 07:54:24 PM UTC, comment #11: 

I don't understand why you would need to be able to create a function handle using @mycls.myfoo or mycls.myfoo.

If we fix the problem of looking up the method lazily, then won't code like your original example work properly when using just @myfoo in a scope where myfoo called with appropriate arguments finds the class method?  In addition, doing lazy lookup will also improve compatibility.

John W. Eaton <jwe>
Group administrator
Tue 29 Aug 2017 06:53:28 PM UTC, comment #10: 

So I think this bug should be closed as Kai Torben Ohlhus propsed a while back.

Piotr Held <jsoh425>
Tue 29 Aug 2017 06:40:05 PM UTC, comment #9: 

I agree this is the wrong approach. I am working on a proper implementation that correctly parses invoking function handles using '@'. After that is complete (if that approach is acceptable) it should be quite easy to ask str2func to evaluate the code it got and return a function handle.

An example of such code would be:


str2func('mycls.myfoo') % or str2func('@mycls.myfoo') %


Piotr Held <jsoh425>
Tue 29 Aug 2017 06:34:52 PM UTC, comment #8: 

I'm not sure what to do with this bug report, but I don't think the proper fix is the approach of the proposed patch.

For compatibility with Matlab, we should probably revisit the lazy evaluation of function handles.

Resetting the status to "None" since I'm rejecting the patch.

John W. Eaton <jwe>
Group administrator
Mon 31 Jul 2017 04:06:51 PM UTC, comment #7: 

Thanks for taking your time to look at my ideas and the few lines of code I wrote!

For my needs the approach you suggested (that is closing this bug) is sufficient. However, I believe that in that case the error message needs to be changed.

With this class:

classdef creating_method_handle &amp;amp;amp;amp;amp;amp;lt; handle
  methods
    function foo (self)
      1;
    end
    function tst1 (self)
      f = @foo; % or str2func('foo') or str2func('@foo')
      f(self);
    end
  end
end


When calling

a = creating_method_handle;
a.tst1();


The response from the interpreter is:

@foo: no function and no method found


This behavior suggests that it should be possible to create a method handle. If it is undesirable to create a method handle anywhere then the error message should be changed as well.

Because of this error message and the fact that someone had implemented parsing for creating a method handle within libinterp/octave-value/ov-fcn-handle.cc:make_fcn_handle() I will bring it up on the mailing list.

Piotr Held <jsoh425>
Mon 31 Jul 2017 10:29:41 AM UTC, comment #6: 

Regarding this item, I think the approach from bug #51599 is preferable, and this one to abandon:

1. It has limited use and works against the idea of OOP, to refer to a specific class method from OUTSIDE that class (comment #4), instead of Octave to find the appropriate `roots` method for the argument given.

2. It encourages "bad" programming style, with lots of references to keep consistent.

3. That it worked with Octave 4.0.3 seems to be more "accidental" than expected (this behavior is not documented).

4. It is not compatible with Matlab.

If you agree, we can stick with bug #51599 and close this item. Otherwise, I leave it open and you can discuss it on the mailing list.

Kai Torben Ohlhus <siko1056>
Group Member
Sat 29 Jul 2017 04:05:17 PM UTC, comment #5: 

I just was trying to get a handle to a method working somehow and I found that this way worked in 4.0.3. So I looked why it doesn't currently work and 'fixed' it. The function libinterp/octave-value/ov-fcn-handle.cc:make_fcn_handle already did the parsing. I just needed to make sure str2func actually ran make_fcn_handle instead of trying (and failing) to create an anonymous function.

I never argued that &quot;lazy evaluation&quot; should be a thing. I just wanted to mention that Matlab does in fact do just that.

I really want to create a function handle to class method, even if the creation happens only within the class scope. So should I bring it up on the mailing list?

Piotr Held <jsoh425>
Sat 29 Jul 2017 03:22:46 PM UTC, comment #4: 

I see, the lazy evaluation was already discussed in bug #31821 and discussed on the mailing list in 2009.  So this behavior will not be added to Octave, I guess.

Regarding your patch.  I find your patch useful in the scenario, that you want to control to use an class overloaded method:


>> noclass = [1 0 -1]
noclass =

   1   0  -1

>> class_p = polynomial (noclass)
class_p =

 1 - X ^ 2

>> fh = str2func ("@polynomial/roots")
fh = @@polynomial/roots
>> roots (class_p)
ans =

  -1
   1

>> roots (noclass)
ans =

  -1
   1

>> fh (class_p)
ans =

  -1
   1

>> fh (noclass)
error: matrix cannot be indexed with .
error: called from
    roots at line 2 column 5


On the other hand, I find this example kind of constructed and is there really a use case for it, instead of calling the class method directly?  Or does Octave encourage programmers to adopt "ugly" indirection patterns? ^^

Kai Torben Ohlhus <siko1056>
Group Member
Sat 29 Jul 2017 12:13:07 AM UTC, comment #3: 

It is not matlab compatible.

As far as I could tell Matlab resolves fcn_handles at runtime so if you have two classes with the same method name (let's call it 'foo') Matlab will let you:


a = class1;
b = class2;
fcn_handle = str2func('foo'); %str2func('@foo') or @foo also work
fcn_handle(a);
fcn_hanlde(b);

and each time the proper method is called for each class.

With Octave the fcn_handles are resolved at assignment so the function you are pointing to must be visible to the interpreter when you create the handle. Matlab only checks if the handle makes sense when you use it.


Piotr Held <jsoh425>
Fri 28 Jul 2017 10:50:52 PM UTC, comment #2: 

Confirmed for classdef as well, before applying the patch to the dev branch:

FOLDER CLASS:


>> p = polynomial ([1 0 -1])
p =

 1 - X ^ 2

>> roots (p)
error: Invalid call to roots.  Correct usage is:

 -- roots (C)

Additional help for built-in functions and operators is
available in the online version of the manual.  Use the command
'doc <topic>' to search the manual index.

Help and information about Octave is also available on the WWW
at http://www.octave.org and via the help@octave.org
mailing list.


CLASSDEF:


>> p = polynomial2 ([1 0 -1]);
>> disp (p)
 1 - X ^ 2
>> fh = str2func ("@polynomial2/disp")
error: Invalid call to disp.  Correct usage is:

 -- disp (X)

Additional help for built-in functions and operators is
available in the online version of the manual.  Use the command
'doc <topic>' to search the manual index.

Help and information about Octave is also available on the WWW
at http://www.octave.org and via the help@octave.org
mailing list.


Afterwards:

FOLDER CLASS:


>> p = polynomial ([1 0 -1])
p =

 1 - X ^ 2

>> roots (p)
ans =

  -1
   1

>> fh = str2func ("@polynomial/roots")
fh = @@polynomial/roots
>> fh (p)
ans =

  -1
   1


CLASSDEF:


>> p = polynomial2 ([1 0 -1]);
>> disp (p)
 1 - X ^ 2
>> fh = str2func ("@polynomial2/disp")
fh = @@polynomial2/disp
>> fh (p)
 1 - X ^ 2


I used the polynomial / polynomial2 classes from the examples directory.

Is this functionality Matlab compatible? I have no chance to check before Monday. I think we should add a regression test to the patch and then it is ready to be uploaded soon.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 27 Jul 2017 12:12:59 AM UTC, comment #1: 

I have written a patch that I believe fixes this problem and am uploading it.

(file #41356)

Piotr Held <jsoh425>
Thu 27 Jul 2017 12:07:47 AM UTC, original submission:  

Although in general creating a function handle fails using the @Class/Method syntax the

str2func('@Class/Method')

still worked as of Octave 4.0.3.

The fix to this seems to be a one-line check. Basically str2func assumes that is the string starts with '@' that means it is a anonymous function. Checking if the next non-whitespace symbol is '(' is can be used for disambiguation of the two cases.

I am attaching a test case that passes on 4.0.3.

In the next comment I will attach my patch.

Piotr Held <jsoh425>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #41356:  patch.diff added by jsoh425 (859B - text/x-patch)
file #41353:  creating_method_handle.m added by jsoh425 (222B - text/x-objcsrc - Test case for the problem)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by jsoh425 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-29 jwe Open/ClosedOpen Closed
    2017-08-29 jwe StatusPatch Submitted None
    2017-07-31 siko1056 Assigned tosiko1056 None
    2017-07-28 siko1056 StatusNone Patch Submitted
        Assigned toNone siko1056
    2017-07-27 jsoh425 Attached File- Added patch.diff, #41356
    2017-07-27 jsoh425 Attached File- Added creating_method_handle.m, #41353

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code