symbolic
symbolic copied to clipboard
gnu-octave
[wip] Change @sym to a classdef.
Fixed issue #545
This is at very early stage. MOST of the tests fail with this change. This is here just for discussion.
I got some error about no class symfun so I thought of converting that to classdef too. Now many of the BIST tests are passing. :)
Hopefully we can have completely working classdefs within a few days...
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 sym class. Do we really want our single classdef to be sooo large? Is there a better work-around? Or should we wait till upstream patches the private functions bug in classdefs? If so, I would be happy to contribute a patch to fix that if anyone could guide me to the file(s) that govern the classdef behaviour...
.
Weirdly, git push is unable to resolve github.com even though I can successfully ping github.com.. I will try to push the changes later...
Did you try defining the function prototype inside the classdef block and defining the method in a function file inside the @sym directory (but not inside a private subdirectory)? This works for me with a test class.
Inside @foo/foo.m:
classdef foo < handle
...
methods (Access = private)
r = some_method (s);
end
And inside @foo/some_method.m:
function r = some_method (s)
...
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 Access = private in the class definition).
Did you try defining the function prototype inside the classdef block and defining the method in a function file inside the @sym directory (but not inside a private subdirectory)?
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. Although I am not sure about protected/private access... I'll check back tomorrow and reply back..
Thanks @mtmiller . I finally found my mistake. I didn't know that to call functions within the same class too, I had to use classname 'dot' and then the function name! This behaviour is different from most of other OO languages... Altough it's fine for now, shouldn't it be allowed to call functions within the same class? Moreover to call a function by itself I have to use 'classname dot function_name'....
.
Almost all the @sym tests pass now. Currently failing tests include symfun tests (all) because I can't seem to figure out how to call 'subasgn' of symfun, because octave automatically calls 'subasgn' of sym (the base class)... Also the tests involving the == operator are failing, though I haven't looked much into them.
.
The test summary for now is:
PASS 1803
FAIL 112
XFAIL 34
. Also, one more concern now. We can't test the private functions!! Is there a way to test private functions in BIST tests?
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 classname.methodname(). Normal instance methods of a class can be called either as method(obj, x, y, ...) or obj.method(x, y, ...)
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.
This is seeing very nice! only a very very very little thing, its possible rename sym.symarray to sym.symcell?? (array normally do reference to matrix, while cell, you know, cells)
You are referring to static methods only, right?
Yes.
we have to keep BIST tests on user-facing functions only.
Sure.
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
Sure it's a way. But adding a 'public test function' to a class... I donno. Thoughts @mtmiller @cbm755 ?
its possible rename sym.symarray to sym.symcell?? (array normally do reference to matrix, while cell, you know, cells)
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 cell_"array"_to_sym which we are wrapping, so symarray seemed equally valid!
But, yes I too don't get the gist of 'array' here...
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 @pyobject/dummy.m to hold the BIST that otherwise get missed due to bugs in Octave (typically the BIST from the ctor). Whatever you do is probably not worse than that ;-)
re: octsympy_tests.m: that should go away soon: its a relic from Octave 3.6/3.8 and should be replaced by runtests or __run_test_suite__.
Woo! Looking good! Why is it a subclass of handle?
No particular reason. Probably passing by reference can help some memory and probably improve performance.. (assuming octave also treats handle subclasses like matlab) Should I make it a value class?
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 subsasgn of the parent class instead of self (in a class of class context).
maybe ask in their forums to know a what do here?
@genuinelucifer why symfun need their own subsasgn if the original don't need it?
a way to use is add subsref and subsasgn as static methods and call it with symfun.subsref.
Thx. Cya.
why symfun need their own subsasgn if the original don't need it?
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. . P.S. - - about the delay, my laptop has stopped working (again) due to hardware issues, i wil resume work on this as soon as possible.
Hi: https://github.com/genuinelucifer/octsympy/pull/5
i send to you this pr to fix subsasgn, but still symfun don't works, always get 0, but the assignment its right, so something else is failing, anyway, step by step.
i had move properties of symfun to sym because octave can't set properties of a subclass...
error: subsasgn: unknown property: vars
error: called from
subsasgn at line 60 column 7
symfun at line 200 column 9
syms at line 176 column 10
subsasgn and subsref need be static to can be called correctly from symfun.
Thx. Cya.
i had move properties of symfun to sym because octave can't set properties of a subclass...
Are you sure? Is it documented anywhere? As I can't test it as of now, I will wait for someone else (or until I find it documented) to confirm this before merging your changes. Or could you do a little test with some barebone classdefs to confirm this?
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:
f.subsasgn (f, idx, vars);
to
f = builtin('subsasgn' f, idx, vars);
and you should have the error i post above.
Thx. Cya.
the next commit its a simplified way to do this, remove the subsasgn of symfun and handle it from sym, you can choose what you like, or check both ways if can help.
Hi all, well with the last commit symfun is fixed!
now, should we overload isa to fix symfun? or just move all isa(a, 'symfun') to isa(a, sym')`?
octave:9> syms f(x)
octave:10> isa(f, 'symfun')
ans = 0
octave:11> diff(f)
ans = (sym)
d
──(f(x))
dx
Thx. Cya.
with the last commit symfun is fixed!
Great :)
should we overload isa to fix symfun? or just move all isa(a, 'symfun') to isa(a, `sym')?
I would say we should have an overload. That seems more correct to me.
Hi, @genuinelucifer its necessary expose symsize?
and @cbm755 can we drop the first and last asserts of subsasgn?
assert( isa(rhs, 'sym'))
assert( ~isa(idx.subs, 'sym'))
assert( ~isa(val, 'sym'))
val.(idx.subs) = rhs;
out = val;
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.
its necessary expose symsize?
Yes. It's actually used in 'size' overload of sym to get the size! :P We can definitely calculate it at every instant via python but that would just be a waste of resources.
Hi, the private properties can be acceded from other functions, test it, seems works if is private:
octave:57> a=sym([1 2;3 4])
a = (sym 2×2 matrix)
⎡1 2⎤
⎢ ⎥
⎣3 4⎦
octave:58> size(a)
ans =
2 2
Thx. Cya.
mm, i found other problem, seems octave don't is calling methods of subclass in files.... maybe this is why subsref can't works... So, for now i think if we continue this, we can move some functions from symfun and handle it from sym..... or move the the functions directly to the symfun subclass. well before send a bug report to octave can someone say me if this code have some error plis?
pipe@Hi ~/Documentos/Cosas/octave/classes $ cat @A/A.m
classdef A
methods
function this = A()
myMethod();
end
end
end
pipe@Hi ~/Documentos/Cosas/octave/classes $ cat @A/myMethod.m
function myMethod()
fprintf('A:myMethod\n');
end
and the octave output:
octave:11> A
error: 'myMethod' undefined near line 4 column 13
error: called from
A at line 4 column 13
Thx. Cya.
Yes, the way you defined myMethod it doesn't look like a method, it doesn't take an object argument, and you didn't call it on this. Maybe you meant either this.myMethod() or myMethod(this)?
Hi, @mtmiller, thx for your comment you are right, i'm little confuse now, because checking the behavior of symfun, now works, but for some reason when we use it, its calling all methods from sym class... now, checking the class:
octave:8> syms x
octave:9> a=symfun(x, {x})
a = (sym) x
octave:10> class(a)
ans = sym
but with the example of the previous comment i can confirm it should be the subclass (should return 'symfun')... Well, we need check weird things.
Thx. Cya. and again thx @mtmiller!
mm, this is very very weird, checking the class before and after of this line:
f@sym(expr);
i get this:
ans = symfun
ans = sym
ans = (sym) x
So, before init the subclass is a symfun subclass, but when we init it its transformed to a sym class................................................. I try this with a little example and it should works, but here is failing..... at least here is the reason why symfun wasn't calling their own methods.
Thx. Cya.
This is what prompted #49421 right? When the sym constructor overwrites the s object, it's no longer a symfun, e.g.
if (nargin == 0)
s = sym(0);
return
end
I asked there for someone to test this kind of constructor chaining in Matlab to see if it's legal. If so, then maybe we need to fix something in Octave, and if not, then is there any recommended way in Matlab to have a base class constructor call itself with different arguments?
I tried many ways to debug this but no result. @mtmiller could you please tell which version of matlab is supposed to support this? I can get a copy of R2013b... Or I can ask one of my friends to try in R2016a if required...
I think any version since R2008 should be a candidate for testing this, but the newer the better for confirmation. Please take a look at the full example on #49421, and maybe stick some debug disps in there to see what the class of the self object is before and after calls to constructors at various places.
So it seems that Octave is Matlab compatible, the problem here is that it's bad practice to have a classdef-based constructor call itself if you are planning on subclassing that object. When a derived class calls a base class constructor, the object that is passed in is still an instance of the derived class. If the base class constructor calls itself and overwrites that object reference, then you've lost the derived instance and broken your class inheritance model.
Do any Matlab experts know a better way to do this? Or is the recommended model to just never have a constructor call itself? Does a constructor just have to have all possible conditional branches to handle different sets of arguments passed to it and fill in the properties of the object conditionally? There is no constructor or method overloading in Matlab, so maybe they figured there is no point to allowing a constructor to call itself.
@mtmiller I forgot to mention on the savannah bug that MATLAB gives an error if I call derived() and the base class constructor calls itself to override the object! Hence it is NOT possible in matlab for a base class to override a derived class object at all! So, in this minor way octave is not exactly compatible. But otherwise yes it seems that this is deprecated to call the constructor within itself.
Ok, thanks for clarifying. So in Octave it's allowed but produces bad results, in Matlab it's just an error. This is what I wanted you to help me discover.
So the sym class ctor will have to be reworked for the classdef implementation.
So the sym class ctor will have to be reworked for the classdef implementation.
Sure. I'll do so and push the changes. This will also eliminate the need of copy which seems good.
.
Also, just for completeness, I get the following error in matlab:
When constructing an instance of class 'derived', the constructor must preserve the class of the returned object
Hi, well i want say i'm little worried about part of this, as i'm seeing this will need a lot of work and rewrite, now, actually i have this pr https://github.com/cbm755/octsympy/pull/594, its practically a rewrite of sym with a lot of fixes to octsympy, i don't like the idea start other rewrite when exist other, its possible in some way before continue merge that pr here or something similar? You know work in this type of things we need time and all that things, and i think do this can help to can start working with a better base.
Thx. Cya.
@latot, perhaps classdef change is going to need changes to upstream Octave (?) so it might be some time until this can be merged to master.
@cbm755 Actually as per the current conventions in both octave and matlab, we should rewrite the sym constructor so that it doesn't call itself! This might lead to more redundant code or more helper functions but it seems the only solution as of now.
Also, @latot 's work in #594 will really be helpful as he has already transferred much of the code to helper functions and hence the rewrite might be easier.
Ok, so sounds like changes in #594 should go in ahead of these classdef changes.
I have based this on #594 and it seems this has some conflicts with master. I will rebase once that pr is merged. The last 2 commits are for this PR. Currently all the tests in @logical and @double pass and we have only 1 failing test in findsymbols in the outer directory because it needs @symfun to be classdef too. I will update other files one by one.
.
Also I am thinking of renaming sym.symarray to something like sym.create_sym because it is now used at a loooot of places and does a lot many things. Any suggestions?
I rebase and merged quite a lot of #594: maybe enough that this could be rebased on top of it?
@cbm755 I have rebased this on top of master. As before, using runtests . in inst folder gives only 1 failing test since symfun is not a classdef yet. I will fix the tests in @sym dir and then convert symfun to classdef tomorrow.
I know it's too late to ask, but by making sym as classdef, we are basically removing MATLAB compatibility! Is this intended? Because if we convert to classdef then all the sym([]), sym({1}) and other similar calls will have to use sym.symarray instead.
While in MATLAB, all the former syntaxes are full correct. Moreover, I saw that in MATLAB sym([1 2]) and sym({1 2}) are same while sym({1 2}) and {sym(1) sym(2)} are different as the former is converted to a sym array with 2 elements while the latter is a cell which has 2 sym objects! We are probably far from fully compatible in this case but still this classdef thing will stretch the gap even farther.
I think your "Moreover" statement is the behaviour we want in both Octave and Matlab... This is #603.
too late to ask
It's never too late to ask.
Personally I have no idea how Matlab sym works, but I do think compatibility is important.
Agree with @cbm755 on #603, and I think that would be supported with classdef, right?
@genuinelucifer what do you think would not work with classdef? Is there a Matlab syntax calling sym that returns something that is not a sym object?
Just to note there are two different forms of compatibility being discussed:
- Running Octsympy on Matlab.
- Octsympy being similar to SMT.
I think @genuinelucifer was referring to 1.
Note no one is working on 1., so it continually bit-rots due to, among other factors, Octave's "embrace and extend" of m-file syntax ;-)
Why don't sym([]) and sym([1 2]) work? Those are supposed to return sym objects, so its not the "sym must return sym" problem... (perhaps I just haven't re-read this thread carefully enough?)
think @genuinelucifer was referring to 1.
I was actually referring to your point 2! Because in MATLAB we can do all of sym([]) sym({1 2}) and similar. While if we change to classdef we can only return 1 sym object and no arrays or cells.
no arrays
This is incorrect, see my comment somewhere else. But yes we'll need to change sym({1 2}) to give (sym) [1 2] 1x2 matrix.
Thanks. I don't know why I did that earlier (giving error at 3 places). We just need error for conversion to cell array. I removed the error from other two places. I'll try to complete this PR with classdefs first. Then we can change the behaviour of cell->matrix in another PR.
Now most of the tests are fixed Except:
- 1 test in
findsymbolsas it requiressymfun - 10 tests in
dsolveforsymfun - 1 test in
isNone. I can't understand why this is failing. But probably we can't use classdef objects for indexing - 5 tests in
subsindex. Again I haven't look closely but seems same asisNone - 2 tests in
symbecause of problem when writing a classdef object usingsave
I have left the above just for my reference and if anyone wishes to test using the code till now. I will update symfun to classdef tomorrow and then target it's tests.
I am stuck now. Actually if I derive symfun from sym (which is in my last commit and I think it should be the right way) then octave always calls subsasgn of sym class in the constructor of symfun class and hence it gives error that the property vars is not found. Moreover, as can be seen, f is sym and NOT symfun in the constructor of symfun from the start. And hence the error!!! I think this has something to do with superiorto call which was earlier present but I removed it as it doesn't work in the constructor of a classdef. I think it is an upstream issue.
.
While if I derive symfun from handle then I can create a symfun but its display is not working and even if I define a display.m and do display(f.sym) it still doesn't work! Well, I didn't try this much as the first way must be preferred in my opinion.
If you explicitly (rather than implicitly) call the superclass constructor, the type of f is at least initially symfun. Something like f@sym(expr) should call the superclass ctor.
But after the superclass ctor, f seems to be a sym; not sure if that is the correct behaviour or not.
Perhaps we need to make a minimal nonworking example to determine if its an octave bug or us?
Ok, I narrowed this down. The issue is that the sym(x) ctor assigns to the input to the output whenever x is already a sym. I think the workaround is that the symfun ctor needs to call the %% The actual class constructor, Tempting to make a private constructor bit.
great! Thanks for narrowing it down. I'll try to fix this tomorrow :)
A little more detail: I had to add symsize to @sym/subsref. Then I changed @symfun/symfun to be:
properties (Access = private)
vars
- sym
end
methods (Static, Access = private)
@@ -156,7 +155,6 @@ classdef symfun < sym
methods
function f = symfun(expr, vars)
- class(f)
if (nargin == 0)
% octave docs say need a no-argument default for loading from files
expr = sym(0);
@@ -208,8 +206,11 @@ classdef symfun < sym
assert (isa (vars{i}, 'sym'))
end
+ class(f)
+ f@sym([], expr.pickle, expr.symsize, expr.flat, expr.ascii, expr.unicode);
+ class(f)
+
f.vars = vars;
- f.sym = expr;
end
Lots of things broken but at least it can make symfun's ;-) Will let you think about this for a bit now. For example, the other approach might be to do this in the case sym(x), but I think this amounts to making copies so perhaps cannot/should not be a subset of handle. Anyway, OO hurts my brain so enough for now ;-)
Thanks @cbm755! Finally symfun is working now :D
Only the tests need to be edited now.
With my latest commit, almost all tests in @symfun dir are now passing! :dancer:
There are just problems with a few tests which I will comment inline as I don't understand why they should pass!
Also, @mtmiller , I think I found a bug with classdefs. The files in a @class folder are not properly updated even after doing rehash or clear classes!! To reproduce do:
- Open octave and cd to
instdirectory of octsympy - In linux terminal do:
mv @symfun/isequal.m ../../ - Then use
isequalon 2symfunobjects. Octave will try to call@symfun/isequal.mand since it doesn't exist, there will be error! I tried doingrehashclear allclear classesbut nothing worked. I had to close octave and reopen it for this to work...
There are quite a few bugs with classdef and @class scoped function files. I'm not quite following what you think the bug is, obviously if you mv the file out of the directory it won't exist anymore. Do you want it to revert to the builtin isequal function in this case? If so, then that looks like https://savannah.gnu.org/bugs/?36230, there's a workaround posted there.
Do you want it to revert to the builtin isequal function in this case?
Yes. As it would if there was no such file to begin with.
If so, then that looks like https://savannah.gnu.org/bugs/?36230, there's a workaround posted there.
Oh. Thanks. That worked for me too.
Finally now most of the tests in symfun are working! I have made a few tests in minus and symvar as xtest since as per me they ought to fail. I can't understand why they aren't failing without classdef.
Also the test in findsymbols is fixed automatically.
But more problems have arrised..... Now dsolve has 14 fails and solve has 10 fails. Both are failing because the equations are automatically evaluated and not being solved as equations and hence they cannot be solved!
Moreover, it seems that, for classdef, callling a(x) where x is the object of sym and a is a normal vector/integer variable is NOT allowed at all. As in such cases the call gives error without going to subsref at all... So there are now failing tests in @sym/inNone and @sym/subsref...
Lastly, there are 2 failing tests in @sym/sym!! One is because, apparently, save cannot be called on a classdef object!! And the other one is some error in assumptions. I marked both of them as xtests. I will look into them later. Currently I am just amazed at how many more errors there are to debug!!! :(
apparently,
savecannot be called on a classdef object
https://savannah.gnu.org/bugs/?45833
One cause of some of the failing tests is because evaluating a symfun at a variable should give a sym:
>> syms x
>> f(x) = 2*x
f(x) = (symfun) 2⋅x
>> f(x)
ans = (sym) 2⋅x
But this PR currently gives a symfun ans(x) = (sym) 2⋅x.
Thanks to @cbm755 , all the tests are passing now! :D
This PR is hence complete and ready for final review, in my opinion. So, please test it and review it once more.
I have just a couple of questions left now:
- What would be the best way to resolve the conflicts without having to rewrite all the changes of this PR on top of master? Till now, to resolve conflicts, I have simply been deleting my branch and creating a new branch from master and rewriting all the required changes. Since this PR spans so many files with little changes in each one, it doesn't look feasible to me.
- Should I squash some of the starting comments? The ones like
Fix almost all symfun tests!andFix more symfun problems?
I would git merge origin/master into this branch, then fix the conflicts.
Before doing that, maybe you could review #731? If we merge that to master, that should make a bunch of the sym.symarray stuff unneeded.
(this classdef change is not going to be merged before 2.5.0 anyway).
I would git merge origin/master into this branch, then fix the conflicts.
Thanks I tried this with meld as mergetool and looks so easy. Good to know this.
this classdef change is not going to be merged before 2.5.0 anyway
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):
- Define
symfunas superior tosymso as to call its functions like that in case ofminus - Allow
saveing ofsymandsymfun - Indexing of constants with classdef is not supported in octave. Either get rid of those tests or leave them as
xteststill octave supports it.
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 xtests. In the mean time, I will try to provide patches for fixing some/all of these points in octave. But, this will still bump the minumum required version of octave for the next release of symbolic to atleast 4.4. I am assuming that it's not a problem...
Re: your points:
- please at least make our class have the inferior classes thing (
classdef {SOMETHING ABOUT sym GOES HERE} symfun < sym) - saving: seems we cannot merge this until this is fixed.
- "Indexing of constants with classdef is not supported in octave": citation needed, do you have an upstream bug report? Definitely leave them as xtests for now. If there is no upstream report, we'll need to make a minimal example and file it.
final review
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.
- defining a classdef superiorto some other classdef
- saving and loading classdef objects
- indexing constants with a classdef object
- https://savannah.gnu.org/bugs/?45833 - load and save on classdef
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?
"indexing constants with a classdef object"
@genuinelucifer do you mean this?
>> a = [1.2 3.4]
a =
1.2 3.4
>> a(sym(3)) = 42.42
a =
1.2 3.4 42.42
@genuinelucifer do you mean this?
Yes, exactly.
Sorry, I've been busy with my exams and couldn't reply.