Rigbox icon indicating copy to clipboard operation
Rigbox copied to clipboard

convert '+vis' package to OOP style implementation

Open jkbhagatio opened this issue 5 years ago • 4 comments

maybe do something like create an abstract class for visual elements, and have, e.g., 'vis.patch', 'vis.grating', 'vis.image', 'vis.grid' be concrete subclass implementations?

jkbhagatio avatar May 02 '19 17:05 jkbhagatio

Classes aren't intrinsically better than functions. I'm removing the 'enhancement' tag until you show that this would be a valuable solution to a problem :P

k1o0 avatar May 03 '19 06:05 k1o0

Setting appropriate abstraction/encapsulation would benefit this package:

[Miles Wells 02/05/19 17:24] In the documentation I try to clearly differentiate between the 
fields/parameters/values/etc. that define a texture (i.e. the structs in elem.layers) and the 
parameters that users can change/define as signals.  The user doesn't really need to know 
about the layers field and they certainly shouldn't assign anything to it.

Inheritance would make this code less redundant and easier to maintain as it would operate more from a single point of control

jkbhagatio avatar May 03 '19 06:05 jkbhagatio

Some things we should consider:

Performance This is very important for Signals. This post is extremely useful btw: https://stackoverflow.com/a/1745686

Although things are improving, I personally think MATLAB is still best suited to procedural programming, especially when it involves basic arrays. I know a lot of Rigbox is OOP and that's great, but it often comes at a cost to performance. Almost all of my analysis is procedural and part of the reason is because loops and function calls are super fast! When Chris first wrote Signals he had to sacrifice a lot of elegance due to speed issues. I think SignalsExp is one of the ugliest files in Rigbox: the rig objects and methods are clumsily pulled into the class because the class method calls were simply too slow. The Window class isn't used at all in Signals for this reason.

MATLAB is improving in this regard and this is another reason to concentrate our efforts on a performance testing framework so that we can keep up with these improvements. It used to be the case that for loops far outpaced cellfun, but now it's viable and more elegant solution. arrayfun on arrays of objects, however, is extremely slow (at least in 2017a). I once added support for Signals arrays be removed it because of shitty performance.

Scalability & Overhead Yes, OOP can make code less redundant and more scalable, however it comes at an initial cost. Refactoring things into classes takes time and for small applications you end up with more code. Instead of a function you have an abstract class and then a subclass, each with their own properties and access definitions. This is fine when you have a real roadmap and know the code will increase in complexity in the future. This brings me to the next point...

Design I think the most difficult part of OOP is having a good design idea. Making a load of classes without thinking through the best way to implement your idea can lead to confusing and ineffective code. We would need to have a good plan before making all these vis functions classes otherwise it'll end up a mess. How much do these functions really have in common? What about them needs to be stateful? What features can't be or aren't already implemented in the current paradigm using Subscriptable Signals to represent the visual elements?

Testing Testing visual stimuli is already challenging, and unfortunately unit testing classes in MATLAB is also very tricky. Testing states and events is difficult and lots of code has restricted access. Unit tests should always be factored into how much time feature implementation will take.

I'm not at all against doing this (in fact maybe it can be made to solve issues like this) but the design plan has to be good and benefits should outweigh the time investment.

k1o0 avatar May 04 '19 15:05 k1o0

Thanks for that link, that's nice. Sure, I agree with most of this, and additionally this isn't high priority. I have cursorily thought about scalability, design, and testing for this idea (I actually think unit testing of '+vis' classes could be done elegantly via MATLAB's unittest framework). I agree performance is an important consideration.

jkbhagatio avatar May 04 '19 18:05 jkbhagatio