search

klabhub / neurostim

Design and run visual neuroscience experiments using Matlab and the Psychophysics Toolbox.
MIT License
5 stars 3 forks source link

Bug fix to log logicals only on value change #222

Open adammorrissirrommada opened 1 week ago

adammorrissirrommada commented 1 week ago

Bug: neurostim.parameter checks for a value change only for numeric and string types, not for logical. So logical values are logged every time they are set, regardless. Is this intended behaviour?

this is a potentially breaking change with wide-reach, because it changes something fundamental related to what is logged.

bartkrekelberg commented 1 week ago

I don't think this was intentional (at least, I cannot think of any reason to do so).

I'd be ok with making this (potentially breaking) change as it can be fixed in the analysis (but more likely nobody relied on this).

If you agree, then #221 can be reverted, as it would now work appropriately without change.

On the logging issue (And related to #224 ) I think there are other scenarios where one may want to log every assignment of a value. The ns.Parameter class could be updated to make that an option (and key presses should probably get that logging option by default).

adammorrissirrommada commented 6 days ago

In the case of #221 , I think I prefer the -1 --> 0 --> 1 sequence, each corresponding to a unique state rather than boolean? What do you think? If you agree, this PR could be merged.

re: key presses and option to always log, I agree. I will put it on my to-do list