portugueslab / stytra

A modular package to control stimulation and track behaviour
http://www.portugueslab.com/stytra/
GNU General Public License v3.0
42 stars 27 forks source link

Stytra (zmq_defix branch) sends wrong experiment duration when GUI params are changed #37

Closed diegoasua closed 4 years ago

diegoasua commented 4 years ago

Duration sent via ZMQ to any microscope acquisition machine (e.g. 2P or lightsheet) is only updated at experiment init. When parameters that affect experiment duration are changed in the GUI this is not reflected in a new experiment duration. This causes acquisition machines to obtain the wrong number of frames.

vigji commented 4 years ago

uh good catch! I guess we were waiting for someone to finally care on the acquisition machine side :)

diegoasua commented 4 years ago

I guess the fix should be easy but I don't know Stytra's architecture well. Duration is sent to ZmqTrigger process through ZmqTrigger.duration_queue. Method check_trigger does not seem to run in a loop? Also not sure where the duration is put into the queue within the software

vigji commented 4 years ago

There's some work to be done to improve the ZmqTriggerclass. It currently also hangs when shutting down an Experimentwith trigger set. It is actuall @OtPrat who was looking into that class, maybe he can have a look and progress that branch to merge in master as it is getting used?

OtPrat commented 4 years ago

I never got deep into that and my knowledge of Stytra's insides is little, but with some guidance I can try to figure it out. Won't get to it until next week though because I have to finish some analysis this one. Is it urgent?

diegoasua commented 4 years ago

It is urgent in the sense that this bug potentially affects any 2P or lightsheet experiment if someone from the lab is not aware of it or forgets about it. Also once this is working fine we should PR and merge with master. I would say it is quite relevant

vigji commented 4 years ago

@diegoasua do you change often the duration of a stytra exp on the fly? I would generally discourage that, and best stytra practices evolved toward not adjusting from the interface stimulus parameters that affect stimulus sequence and would determine a new "version" of the experiment

diegoasua commented 4 years ago

Completely agree, however then what is the use of those params? If the can't be modified. I think we should encourage not accesing through GUI and making new versions oc. But there are cases in which people might end up very confused. For example you use an empty Stytra protocol for spontaneous activity, you will probably change duration from GUI. If you hard-type it in the code then master branch has uncommited changes and next version can't be pulled. I guess the point I want to make is that yes, that would work, but it is a dirty solution. Whereas changing this duration queue, if one knows Stytra's architecture, should be almost trivial

vigji commented 4 years ago

define Params is good for logging, there's an option editable=false that you can pass to show them disabled in the gui. Having them editable is still good in some scenarios (e.g., adjusting gain per-fish, or stuff like that). Of course though, this is a bug and should be fixed :D

vilim commented 4 years ago

I disagree with @vigji's point about never changing the variant of the protocol in the GUI, I think that you should maintain the same protocol name as long as it can be analysed with the same code (which should always be that case as long as you are changing only the GUI-available parameters). Some experiments might allow fewer some more repetitions, but making a new protocol variant for that is completely superfluous.