-
Notifications
You must be signed in to change notification settings - Fork 37
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] Change @sym to a classdef. #590
base: master
Are you sure you want to change the base?
Conversation
@@ -65,7 +65,7 @@ | |||
|
|||
% Note: symbolic sized matrices should return double, not sym/string. | |||
|
|||
n = x.size; | |||
n = x.symsize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was required. Because if I used size
as property in the sym
class then too (weirdly) octave called the size
function and did not use the property..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that because @sym/subsref
explicitly forbids public access of x.size
?
I got some error about |
inst/@sym/horzcat.m
Outdated
@@ -62,7 +62,7 @@ | |||
'return sp.Matrix.hstack(*_proc),' | |||
}; | |||
|
|||
varargin = sym(varargin); | |||
varargin = cell_array_to_sym(varargin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change (and other similar ones) are required because after converting sym
to classdef
, octave is not allowing to call the constructor with a cell array!!!
That is, sym({1})
fails!!! I tried with some test classdef classes but the same error didn't appear. Something is different/weird with respect to sym
only!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm, this is weird, in my case works fine cells with sym, in my octave the classdeff can't access to the private functions....
octave:12> sym(2)
error: 'magic_double_str' undefined near line 230 column 19
error: called from
sym at line 230 column 17
maybe move the private files functions need sym to sym in private functions?
list of functions need sym:
*cell_array_to_sym
*numeric_array_to_sym
*magic_double_str
what version of octave are you using? (i'm using 4.2.0-rc2)
we probably need report this things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I got that error message too. I just added the private dir to path... I had faced similar issues in past but could never figure out the cause. So i thought it was not related to classdef.
And, after classdef, did u try 'sym({1})'?? For me octave gave the error' cannot convert cell to object'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I am using 4.2-rc2, built from source downloaded from the gnu alpha website.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm, seems i misinterpret a little why you change sym to cell_array_to_sym, yes it don't works, but we can call normally the classdef with cells but the private function for some reason can't be called:
octave:1> sym({'f'})
error: 'cell_array_to_sym' undefined near line 210 column 13
error: called from
sym at line 210 column 11
i try adding the function as private in the classdef but seems don't is supported for octave...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of anything off the top of my head, please continue to debug to find out why this is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it was pretty easy to debug this. Notice that the @sym/sym
constructor is returning a cell array of sym
objects. The error occurs after the @sym/sym
constructor returns, if you set breakpoints and step through the code it's very easy to see this.
My guess is that Octave expects a classdef
-based constructor to always return an instance of the object. What you have here is either a weird corner case or an abuse of the language, in the old-style constructor which returns a cell array instead of an object of type sym
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOW, I think the current situation of
my_syms = sym ({'x', 'y', 'z'})
is neither good form nor does it seem to be legal under classdef
-defined classes. A call to sym (anything)
should always return an object of type sym
.
Using classdef
, it might be better to define a static method such as
my_syms = sym.symarray ({'x', 'y', 'z'})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it sounds better to be that way. But again, we have some odd conversions in place which might make it difficult (or rather would required many conditionals)....
Thanks for the debug.. I actually tried to debug it with breakpoints but the breakpoints could not be put on a classdef class.... I just read on the wiki that breakpoints via gui are not yet supported for classdef and have to be put via the cli....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed #603 for this cell = sym(cell)
thing.
inst/@sym/sym.m
Outdated
s = sym(0); | ||
return | ||
classdef sym < handle | ||
properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make some or all of these properties Access=Protected
or Private
? Some are used outside of the class but many are only used directly inside class methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, i try it but didn't works D: i get the message can't found the function...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, currently only symsize
is used outside the class. I will check to make sure and then keep others as Protected (just to allow symfun to use them if needed, assuming that derived class can use protected members of the base class in the classdef style too)....
Hi, it seems that there is no way to define private functions in separate files (even within the same folder). Also, I ended up copying some of the functions from private folder directly into the |
Did you try defining the function prototype inside the Inside
And inside
This is the "official" way of defining private methods in external files on Matlab's site, there is no mention of private subdirectories there. Not saying the private subdir way shouldn't be supported or that it's wrong, but this technique seems to work in Octave today (just move all methods up a level from private, and tag them as |
I think yes. I actually copied all the files in private directory to the @sym directory. And then yes I included the prototype in the class. I played around with many different permutations of files in same directory or in private and different combinations of access of the methods. |
Thanks @mtmiller . I finally found my mistake. I didn't know that to call functions within the same class too, I had to use
. |
You are referring to static methods only, right? Yes, that is the way the language works, whether inside or outside of the class, a static method is called with There is no way to test private functions. A BIST test is just like something you would run at the command line, and the problem is that the private function is not visible outside of its scope. So we have to keep BIST tests on user-facing functions only. |
mm, some way is make a new public function of the classdeff with the function of test private functionalities and send an assert if don't works. |
inst/@symfun/symfun.m
Outdated
end | ||
|
||
idx.type = "."; | ||
idx.subs = {"vars"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ups remember here simples quotes .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. These are my last second changes before pushing and moreover don't work as expected. I'll change this in the next commit. Thanks for pointing out.
This is seeing very nice! only a very very very little thing, its possible rename |
Yes.
Sure.
Sure it's a way. But adding a 'public test function' to a class... I donno. Thoughts @mtmiller @cbm755 ?
Sure, I have no problem with the rename. I just picked out the name from one of Mike's comments. Also there is the function |
only as a note,actually is available to the user the function octsympy_tests, so don't think its too crazy add a test function. |
Woo! Looking good! Why is it a subclass of handle? Also re: BIST. In Pytave, I did a re: |
No particular reason. Probably passing by reference can help some memory and probably improve performance.. (assuming octave also treats handle subclasses like matlab) |
If I'm reading the code right, sym objects are immutable, right? So making it a value class would incur unnecessary copies of the internal properties for no added benefit. I think handle makes sense, unless I missed a spot where a sym can be modified in place. |
Hi, only to be sure, Matlab have this behavior?, i refer to call |
yes, I think Matlab has the same |
maybe ask in their forums to know a what do here? @genuinelucifer why symfun need their own a way to use is add Thx. Cya. |
We because in the constructor of symfun if we try to assign value to a property then the subsasgn of 'sym' is being called which simply breaks on a assert statement. I didn't want to change behavior of sym class so though to implement it for symfun too. |
Hi: genuinelucifer#5 i had move properties of symfun to sym because octave can't set properties of a subclass...
Thx. Cya. |
Are you sure? Is it documented anywhere? |
Hi, sadly when i try reproduce it with a simplified version i can't do it works (i don't know to much about classdef), so for now to confirm it, someone else can change this line in symfun:
to
and you should have the error i post above. Thx. Cya. |
the next commit its a simplified way to do this, remove the |
Hi all, well with the last commit symfun is fixed!
Thx. Cya. |
Great :)
I would say we should have an overload. That seems more correct to me. |
Hi, @genuinelucifer its necessary expose and @cbm755 can we drop the first and last asserts of
How now we are working with classdef i think we can use the properties more free, and how the prop are privates if the user try change someone octave will send the error, and if try set a non existent property octave will send an error too. Thx. Cya. |
Yes. It's actually used in 'size' overload of sym to get the size! :P |
Hi, the private properties can be acceded from other functions, test it, seems works if is private:
Thx. Cya. |
With my latest commit, almost all tests in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the tests are working now. If I can understand things in these tests then I will fix them and then look at the tests that I left in @sym
unattended... Hopefully this PR will be complete soon :D
inst/@symfun/minus.m
Outdated
%! h = f ^ x; assert(isa(h, 'symfun') && isequal(h.symbol, g ^ x)) | ||
%! h = f .* x; assert(isa(h, 'symfun') && isequal(h.symbol, g .* x)) | ||
%! h = f ./ x; assert(isa(h, 'symfun') && isequal(h.symbol, g ./ x)) | ||
%! h = f .^ x; assert(isa(h, 'symfun') && isequal(h.symbol, g .^ x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since earlier it was sym - symfun
so minus
of sym
was being called, so I changed it to symfun - sym
to allow for proper call of out function. I don't know how the conversion/calling worked without classdef. If required, we can add an if statement in sym
to convert results of sym - symfun
to symfun
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests should stay sym - symfun
: the whole point of this test is the Octave bug #42735. You can mark it xtest
if you can't make it work rather than changing it.
I notice the older workaround gives infinite recursion so we may need a new workaround! In the workaround in @sym/plus
, I tried plus@symfun(x, y)
but that gave an error message than symfun
is not a subclass of sym
(which is true enough).
In some cases, such as ``@sym/minus.m` we could do:
% Dear hacker from the distant future... maybe you can delete this?
if (isa(y, 'symfun'))
warning('OctSymPy:sym:arithmetic:workaround42735', ...
'worked around octave bug #42735')
z = plus(-y, x);
return
end
(but I don't know we can do that for every case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in minus atleast, we are calling minus on the symbol
of symfun and the sym and then converting the result to symfun. If it's ok I can add conditions in minus
of sym
so that it converts the result to symfun
if either of the operands is symfun
.. Is that ok? Or should I mark it as xtest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover it seems that superiorto
can not be called in a classdef! So, I don't know if we can fix this with superiorto even if upstream is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not called "superiorto" in classdef: I've posted at the [upstream bug] (https://savannah.gnu.org/bugs/?42735).
@genuinelucifer Maybe you're the right person, at the right time, at the right place to try to fix this upstream bug? I'm sure @mtmiller would be very willing to review a patch for such thing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's ok I can add conditions in ...
Dunno, try I guess. My impression was if we cannot call the subclass method than all the subclass code will have to be duplicated in @sym
... ick! Fix upstream would be best of all of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you're the right person, at the right time, at the right place to try to fix this upstream bug?
Sure. After finishing this PR, I will try to submit a patch upstream...
inst/@symfun/mtimes.m
Outdated
@@ -58,7 +58,8 @@ | |||
|
|||
function h = mtimes(f, g) | |||
[vars, s1, s2] = helper_symfun_binops(f, g); | |||
h = symfun(s1 * s2, vars); | |||
res = s1 * s2; | |||
h = symfun(res, symvar(res)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is most confusing part for me... When we are doing any arithmetic operator. The result will definitely have variables from both the symbols involved in that operation. In helper_symfun_binops
, it's not allowed to do operations on 2 symfuns if they dont' have exactly same variables!! Also, the variables of the symfun
are only considered as final variables in any operation!! This definitely looks wrong to me. I was tempted to change helper_symfun_binops
to take a union of the sets of variables in each of the operands involved but I currently did this only for mtimes
.
Am I missing something? Why isn't operations for symfuns with different variables allowed? Why are variables only from the symfun operand considered as variables in final expression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find a reference but IIRC, SMT did this too. I agree it does seem reasonable to take the union of the inputs as the vars of the result. But anyway this has little to do with this PR right?
The result will definitely have variables from both the symbols involved in that operation
Hmm, but this may not be the correct result. Consider:
octave:9> f(x,y) = 2*x
f(x, y) = (symfun) 2⋅x
octave:10> g(x,y) = 1
g(x, y) = (symfun) 1
octave:11> f*g
ans(x) = (symfun) 2⋅x
(Previous to this PR, that would be ans(x,y)
.)
Also, the variables of the symfun are only considered as final variables in any operation!!
Not sure what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway this has little to do with this PR right?
Well, partly yes. But some tests are failing because they are not getting correct vars in result. Such tests are very less in number so I can leave them now and fix them in next PR.
Hmm, but this may not be the correct result. Consider
Well, in the example you gave, the previous approach would work because both are symfun
and both have the same variables.
Not sure what you mean
If we consider a symfun f(x) = x^2
and a sym y
then f(x)*y
should have both x
and y
in its vars
. But since the code of helper_symfun_binops
considers only the variables of the symfun involved in such cases,the result will have only x
and not y
included in the vars
.
I see that my current approach of symvar
is wrong too. I can change it to take union of the variables from both the operands involved. I think that would definitely work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it to take union of the variables from both the operands involved.
Let's take this conversation to #738. I don't see it being relevant to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
inst/@symfun/diff.m
Outdated
@@ -78,7 +78,7 @@ | |||
|
|||
function z = diff(f, varargin) | |||
|
|||
z = diff(f.sym, varargin{:}); | |||
z = diff(f.symbol, varargin{:}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I could not find any direct way of converting a classdef to its parent class, I had to store expr in a symbol
property. Also, If I name it sym
then it doesn't work! So I named it symbol
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe in this case you could call diff@sym(f, varargin{:})
. That is supposed to be the proper OO syntax for dispatching to a base class method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks! Will do.
Also, @mtmiller , I think I found a bug with classdefs. The files in a
|
There are quite a few bugs with classdef and |
Yes. As it would if there was no such file to begin with.
Oh. Thanks. That worked for me too. |
Finally now most of the tests in Also the test in But more problems have arrised..... Now Moreover, it seems that, for classdef, callling Lastly, there are 2 failing tests in |
|
One cause of some of the failing tests is because evaluating a symfun at a variable should give a sym:
But this PR currently gives a symfun |
inst/@symfun/subsref.m
Outdated
@@ -43,19 +43,20 @@ | |||
|
|||
switch idx.type | |||
case '()' | |||
out = subs(f, f.vars, idx.subs); | |||
out = subs (f.symbol, f.vars, idx.subs); | |||
out = symfun (out, symvar (out)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this out = symfun (out, ...)
is incorrect.
I'm not sure how to do this properly, but symfun should support casting to sym. Make "formula" use this new casting.
I'm not even sure this cast was correct for old-style class but its certainly not for classdef. Anyway, "formula" is semantically correct in these cases.
Fixes for casting to sym
Thanks to @cbm755 , all the tests are passing now! :D I have just a couple of questions left now:
|
404672b
to
336eddf
Compare
I would Before doing that, maybe you could review #731? If we merge that to master, that should make a bunch of the (this classdef change is not going to be merged before 2.5.0 anyway). |
Thanks I tried this with
Oh ok. Then I would wait for that as the new master will have to be merged again anyway. Also, Currently we have following 3 outstanding errors due to use of classdef (just to sum it up):
I am assuming we will translate to classdef hoping that all these will soon be supported by octave and we will get rid of all the |
Re: your points:
Wishful thinking ;-) It seems quite likely we will not merge this until 4.4 is released :( |
I think master is reasonably stable; would be good to merge to here. We also need a clear idea of what needs fixed in upstream octave to get this merged. |
Well, the 3 things that I mentioned in my last comment seem to be the bottleneck currently.
|
Do those all have upstream issue numbers? |
I am not sure what is meant by "indexing constants with a classdef object". Does this mean using a classdef object as the index of a subsref on a builtin matrix? Is there some builtin conversion rules that are supposed to be used? Do we have a Matlab doc describing this? I thought there were some other bugs. Are there other classdef issues that you were able to find workarounds for, but it would still be better to not have to use a workaround? For example method name resolution problems, problems with private methods, etc? |
@genuinelucifer do you mean this?
|
Yes, exactly. Sorry, I've been busy with my exams and couldn't reply. |
Fixed issue #545
This is at very early stage. MOST of the tests fail with this change. This is here just for discussion.