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

 
 

bug #51586: Creating method handle fails using str2func

Submitted by:  Piotr Held <jsoh425>
Submitted on:  Thu 27 Jul 2017 12:07:47 AM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Regression
Status: NoneAssigned to: None
Originator Name: Piotr HeldOpen/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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:

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>
Project 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:

When calling

The response from the interpreter is:

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>
Project 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:

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>
Project 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:

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:

CLASSDEF:

Afterwards:

FOLDER CLASS:

CLASSDEF:

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>
Project 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
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 File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by siko1056 (Posted a comment)
  • -unavailable- added by jsoh425 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 29 Aug 2017 07:54:24 PM UTCjweOpen/ClosedOpen=>Closed
    Tue 29 Aug 2017 06:34:52 PM UTCjweStatusPatch Submitted=>None
    Mon 31 Jul 2017 10:29:41 AM UTCsiko1056Assigned tosiko1056=>None
    Fri 28 Jul 2017 10:50:52 PM UTCsiko1056StatusNone=>Patch Submitted
      Assigned toNone=>siko1056
    Thu 27 Jul 2017 12:12:59 AM UTCjsoh425Attached File-=>Added patch.diff, #41356
    Thu 27 Jul 2017 12:07:47 AM UTCjsoh425Attached File-=>Added creating_method_handle.m, #41353

    Back to the top


    Powered by Savane 3.1-cleanup1