str2fun

[adammorrissirrommada]

@bartkrekelberg I've pushed a working version of the new str2fun format (behaviorDemo and sog work for me). Each function property get() takes 0.078 ms for me; that's 40% of the time it used to, a substantial increase in performance.

The regular expressions in str2fun might need some cleaning up - I'm hopeless at those, so have a look!

Also, why we are we logging the value returned by getFunctionValue() every time it is called? I noticed you said somewhere that checking whether property values had changed was taking longer than just logging everything, but that was for a set() operation. Perhaps it would be ok to do such a check for the returned function values and only log them if they change?

[adammorrissirrommada]

This approach still has some problems.

1) Only plugin properties that were added with addProperty() can be referenced in a function property (because only they are neurostim.parameter objects). This isn't a deal-breaker, but it's not ideal.

2) str2fun sets up an anon fun that reads the "value" field of the neurostim.parameter, but this isn't right. This isn't a get() of any kind. It doesn't work when a function property includes a reference to another function property, because in that case it should be calling getFunctionValue(o) rather than just reading the value property. Perhaps this could be solved by neurostim.parameter having a get() function that knows whether it should return o.value or getFunctionValue(o) (e.g. by checking if o.fun is empty)... but then maybe this would cause too much overhead.

This led me to wondering why it is necessary to have the dynamic prop and its neurostim.parameter mate. I recall there being a good reason, but it escapes me now. Can @bartkrekelberg remind me?

I'm keen to solve this, because I'm getting a substantial improvement in performance .

[adammorrissirrommada]

To clarify point 1 above, you can include anything in a function property (e.g. rand, sqrt, +/-, etc), but if it is a property of a plugin (o.prop), it has to be one added with addProperty(). It can't be one defined in the class definition.

[bartkrekelberg]

This led me to wondering why it is necessary to have the dynamic prop and its neurostim.parameter mate. I recall there being a good reason, but it escapes me now. Can @bartkrekelberghttps://github.com/bartkrekelberg remind me?

The dynprop is defined to allow users to write

o.X

in their code and get the value.

An alternative approach may be to define a subsref function for plugin that handles the . subsref for all parms by passing it to the parameter.

In other words addProperty would define a neurostim parameter in prms and o.X would call plugin.subsref with ‘X’ which would call

o.prms.X.get()

get() would check whether to evaluate a function or not (just as it is done now for o.X dynprop calls in the GetMethod).

Hard to predict whether this will be faster, but it could be.. and it removes the odd mirroring of dynprops and parameters.

[adammorrissirrommada]

If I understand correctly, this would probably not work, because only some plugin properties should be neurostim.parameters. Others are just private/protected/predefined properties, not logged, not functionable etc. I guess we could do a check in the subsref but then maybe that would end up being slow?

[bartkrekelberg]

Yes, you’d have to check whether the property is in o.prms , if not return the member

If isfield(o.prms,prop) Value= o.prms.prop.getValue(); Else Value = o.(prop); End

Sure there is some overhead, but it may be less than what we currently have…

From: Adam Morris [mailto:notifications@github.com] Sent: Wednesday, June 21, 2017 7:36 AM To: klabhub/neurostim-ptb neurostim-ptb@noreply.github.com Cc: Bart Krekelberg bart@vision.rutgers.edu; Mention mention@noreply.github.com Subject: Re: [klabhub/neurostim-ptb] str2fun (#72)

If I understand correctly, this would probably not work, because only some plugin properties should be neurostim.parameters. Others are just private/protected/predefined properties, not logged, not functionable etc. I guess we could do a check in the subsref but then maybe that would end up being slow?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/klabhub/neurostim-ptb/issues/72#issuecomment-310051611, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ALbI0zdRrCevLV5ty7JyUaty2kA7Gwqkks5sGQAtgaJpZM4N8ION.

[adammorrissirrommada]

I don't suppose it's possible to make neurostim.parameter a derived class of meta.DynamicProperty, such that you can add the neuro param directky to the plugin with addprop()? I suspect Matlab doesn't allow this...

[adammorrissirrommada]

str2fun is also used in block.beforeFunction, but that isn't a plugin so I think I probably broke this functionality in my revised version. I'll look into this when I can.

[bartkrekelberg]

Good news first. Logging only changed values is faster than logging all. I committed that (not sure why it was slower earlier on...)

Then I looked into using subsref instead of dynamicprops. There are various complications, but the show stopper (for me) is that because subsref is not called for code in the class, the class developer would have to call subsref explicitly. IN other words in a member function you could not write o.myProp = 5 but would have to write some hard to read code using subsasgn.

So I think that kills that idea.

I will try to make something that addresses points 1 and 2 above..

For #2 Checking for an empy .fun won;t cost much time (we could even store a bool to make this faster).

1 is trickier.

[adammorrissirrommada]

FYI: your change brings my home PC to 0.047ms per call (as reported by speedTestForFunctionProps). That's excellent!

Regarding #1: Perhaps there would need to be some sort of hybrid str2fun that is somewhere between this one and the old one: the "args" stores parameter handles where possible and plugin handles otherwise? That would probably be fast because I think accessing through plugins was slow earlier mainly because it was indirecting all the way to the parameter object down the tree... accessing a normal Matlab property of the handle would be as quick as it is for our ns params we are using now.

What do you think?

[adammorrissirrommada]

I just committed such a version. Seem to work and is very fast (0.027ms per call).

By the way, all current versions work with forms of o.this, but not with multiple levels of nesting, for example, c.screen.width. But that's ok.

Nor things like:

o.X = '@dots.dx(3)'

[bartkrekelberg]

Great.

I think current the str2fun is called every trial Possibly because setDefaultsToCurrent puts the '@nnn' string back as the value, which then gets converted to a function handle again,

There could be advantage to this, and the downside is not too big (ITI is usually not time critical ).

Or is this needless recomputation?

Other than that, I think this could be merged back into the master.

[bartkrekelberg]

We should keep the recomputation: that way variables can be functions insome conditions but not in others.

[adammorrissirrommada]

Something to be aware of:

I found that the time taken to do this:

if o.this

is maybe 10x slower if o.this is specified as a logical property in the definition (i.e. this@logical) than if it is just left as a generic Matlab variable (i.e. double). I guess Matlab is optimised for numbers and does some sort of costly conversion to pretend it has logicals. Weird.

[adammorrissirrommada]

I think I lost some performance in the end, relative to the intermediate (though ultimately incorrect) version that just read the .value field. It's now calling getValue() (so it should) and it gets called billions of times and it all adds up. I did a quick comparison of the old master (before we re-jigged str2fun) and this new one and it was quite a bit better... but overall, I think we need to work on performance more... particularly in stimulus.baseBeforeFrame()..

[bartkrekelberg]

I tried one more change: most of the time in baseBeforeFrame is spent reading .x .y etc but these change rarely. So I created a member variable (.transform) that contains all of these (.x .scale etc) as a single vector. Whenever one of them is changed (in parameter.m) they update the stimulus.transform. The baseBeforeFrame then only reads the .transform (Which is fast). While this speeds things up, it also breaks NS functions, because they only update when used. In other words, when s.X is a function, it should update on every frame, but because we only check s.transform in baseBeforeFrame it does not update, so we keep using old values. (Easily seen in demos/gazeContingent). So this solution, while fast, does not work.

We could save some time by specifying XYZ as a single vector instead of three separate properties: 2 fewer calls per frame, which is 0.06 ms for a static but 0.22 ms for a function (on my machine). Downside is that writing code in which X is varied only becomes much more cumbersome. Not sure that is worth it. For now, I think we've reached the limit of optimization in this bit of code...