Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] Implement @sym/class to return python class name of @sym vars #561

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

genuinelucifer
Copy link
Contributor

fixes issue #549

Currently it does not follow octave's builtin class symantics. I have kept a full flag just for discussion.

@genuinelucifer
Copy link
Contributor Author

genuinelucifer commented Sep 20, 2016

it breaks most of the doctests currently. I'll update the doctests once this is finalised.

%% Return class name of the variable x.
%%
%% @var{full} decides if the fully qualified class name will be returned
%% or not. It it true by default.
Copy link
Contributor

@latot latot Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very very little thing, i think its "It its" or "Its"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.. Thanks for pointing out, I'll fix the typo.

@latot
Copy link
Contributor

latot commented Sep 20, 2016

Hi, this is very nice!
it seems works fine with syms, i think left symfuns, you will need improve this to can set class (values and all that things):

>> syms g(t)
error: Invalid call to class.  Correct usage is:

 -- Method on C: = class (X)
 -- Method on C: = class (X, FULL)
error: called from
    print_usage at line 90 column 5
    class at line 51 column 5
    symfun at line 184 column 5
    syms at line 176 column 10

from symfun.m line 184:

  f = class(f, 'symfun', expr);

sadly we can't use the builtin function in symfun:

  f = builtin('class', f, 'symfun', expr);

octave don't likes this, don't recognizes the call as the type (@symfun/symfun):

>> syms g(t)
error: class: 'symfun' is invalid as a class name in this context
error: called from
    symfun at line 184 column 5
    syms at line 176 column 10

its very probable this can be a octave bug...

And maybe instead of this line:

(nargin == 2 && !islogical(full))

remove it and change the python cmd line to:

cname = python_cmd (cmd, x, logical(full));

i think its very normal use 0 and 1 as logical values, only to enable that.

Thx. Cya.

%!error <Invalid> class (sym(1), 2)

%!test
%! syms x y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi its possible write instead of this line:

syms f(x, y)

?, to test sym and symfun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohk. Or maybe we can have additional tests for symfuncs... This way, just to be sure, both can be tested separately. Will add a few more tests..

@genuinelucifer
Copy link
Contributor Author

i think left symfuns, you will need improve this to can set class (values and all that things):
i think its very normal use 0 and 1 as logical values, only to enable that.

Hi @latot . Sure I'll incorporate these changes.

@genuinelucifer
Copy link
Contributor Author

I just wanted to note that I tried a few different things but as I am not able to find a decent way to make it work for both syms and symfuns...
symfunc calls class at one point in the constructor and if I make a @sym/class.m then that is called. Which causes the creation of symfunc to fail. Also, in python I am getting a dict when I pass the symfunc! I probably have lost track of how symfuncs are being created... Moreover if I return any string from class say using:

if (isa (x, 'symfunc'))
   cname = 'symfunc';
end

then if I try syms g(t) then g is actually the string symfunc!!
Also, as @latot mentioned, calling builtin('class',... also seems to be not working!
.
This probably would be a lot easier to fix on pytave IPC. If I don't get a way around this in windows, I will update the file to only work with pytave IPC and directly return the pyobject instead (which I hope should be fairly straightforward)...

@cbm755
Copy link
Collaborator

cbm755 commented Sep 27, 2016

  1. Does implementing @symfun/class help? (I guess not)
  2. The ctor call to class may not be with classdef... Maybe we can do this class override after we switch to classdef?
  3. We should take the f = builtin('class', mystruct, 'symfun', myparentobject) thing to upstream Octave. Can you explain it or do you want me to file it?

@latot
Copy link
Contributor

latot commented Sep 27, 2016

I already report it, if you can complement the explanation it would be great:

https://savannah.gnu.org/bugs/?49169

Thx. Cya.

@latot
Copy link
Contributor

latot commented Sep 27, 2016

The idea its can overload class only to return a value, don't to set (when we will set we call builtin).

@mtmiller
Copy link
Collaborator

https://savannah.gnu.org/bugs/?49169

Commented upstream, hopefully we can get some information on what Matlab does in this situation.

Does implementing @symfun/class help?

No, the error is in overload resolution and the extra arguments passed when doing inheritance with old-style class-based classes.

The ctor call to class may not be with classdef

Right, classdef would work around this.

@latot
Copy link
Contributor

latot commented Sep 29, 2016

mm, personally i think this @sym/class helps a lot, i normally works with a mix of things, syms, symfuns, intervals and matrix, and when i display the info its too hard know what are matrix and what are intervals(when i have a lot of data)....

@genuinelucifer
Copy link
Contributor Author

Right, classdef would work around this.

I will then try to do the classdef implementation now. If that works then we could implement this on top of that.

@latot
Copy link
Contributor

latot commented Sep 30, 2016

@genuinelucifer and @cbm755 a little question, i don't have very clear what are differences between the actual sym and implement classdef (i try search but i can get a clear answer), ex. why class should works from classdef and not now? or what the differences between the concepts vs functionalities?

The mains differences i have clear are, you can set methods from classdef (but we can do this too from syms in other files), in classdef you can set properties to the objects (in some way sym do this with a struct), the extras of classdef are the events and enumeration but this last two don't are available in octave (and i was start thinking how use it :D).

This can approached from other way, edit the display function and add the actual class code there.

Thx. Cya.

@cbm755
Copy link
Collaborator

cbm755 commented Sep 30, 2016

I'm not clear on differences either. I think of them as two different approaches to OO, or at least two different implementations. And so there are some differences between them, different bugs too ;-)

Classdef-style classes allow more OO features like static methods.

@cbm755
Copy link
Collaborator

cbm755 commented Sep 30, 2016

@latot where you working on the classdef? @genuinelucifer if you start too, let's get a "WIP" PR up ASAP for collaboration and not duplicating effort.

@latot
Copy link
Contributor

latot commented Sep 30, 2016

I'm not working on it now, for i can help i need first understand the concept.

Thx. Cya.

@cbm755
Copy link
Collaborator

cbm755 commented Sep 30, 2016

Thanks @latot: your review if @genuinelucifer does it would be very much appreciated (and good chance to learn).

@genuinelucifer
Copy link
Contributor Author

if you start too, let's get a "WIP" PR up ASAP for collaboration and not duplicating effort.

Definitely. I am compiling octave on my linux machine. I can probably send a WIP PR by tomorrow.
I also have to go through classdefs once.

@latot latot mentioned this pull request May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants