AllenNeuralDynamics / dynamic-foraging-task

Bonsai/Harp workflow for Dynamic Foraging with Python GUI for visualization and control
MIT License
5 stars 4 forks source link

Update random method #543

Open XX-Yin opened 1 week ago

XX-Yin commented 1 week ago

Pull Request instructions:

Describe changes:

  1. Enable to set the randomness methods (even and exponential distribution) separately for block length and ITI/Delay.
  2. Updated the NWB file to include the randomness fields.
  3. The uncoupled task hard-coded the randomness methods for block length as even. The update allows us to control the randomness of the uncoupled tasks from the GUI.

What issues or discussions does this update address?

https://github.com/AllenNeuralDynamics/dynamic-foraging-task/issues/542

Describe the expected change in behavior from the perspective of the experimenter

  1. The curriculum may need updating in the future (@hanhou). The Randomness was moved and updated with two fields called RandomnessBlock and RandomnessOther. If not provided, RandomnessBlock and RandomnessOther will default to Even and Exponential respectively.

Describe any manual update steps for task computers

No

Was this update tested in 446/447?

XX-Yin commented 1 week ago

@KanghoonJ @ZhixiaoSu

The block distribution of the uncoupled task was set to Even by default (hardcoded by @hanhou). I updated the code to explicitly set the block distribution and ITI/delay distribution independently for all tasks.

XX-Yin commented 1 week ago

@hanhou @alexpiet @ZhixiaoSu @KanghoonJ This is ready for review.

hanhou commented 1 week ago

Can you explain more about:

  1. Why do we want to use exponential distribution in the uncoupled task? I would imagine the effective block size will be much shorter if the block length on each side is further left-skewed due to the exponential distribution.
  2. Why do we want to use uniform distribution in the coupled task? I think having a flat hazard rate is still desirable in the coupled task.
XX-Yin commented 1 week ago

2. hazard

Hi Han, the idea is to allow us to set the distribution of all tasks from the GUI.

  1. I agree to use the uniform distribution for block length in uncoupled task.
  2. For coupled tasks, it may be better to use a uniform distribution, too. 1) It will let us get more longer blocks during analysis. 2) If it's left-skewed exponential distribution, animals may perceive the block more as short blocks and develop strategies good for short blocks.
hanhou commented 1 week ago

If your main concern is that blocks are too short, you can increase the min_block; if you concern is they are too left-skewed, then you can increase beta to make them more even. I think making the block uniform is a major change to the task, and we should discuss it more thoroughly beforehand. Could you add a discussion point to the next behavior meeting?

Also, changing the name will break lots of downstream pipeline, especially considering that during the production_test phase there will be different rigs running on different codebases. Could you still use Randomness for blocks (default = exponential) and RandomnessOther for ITI etc (default = exponential too)?

XX-Yin commented 1 week ago

@hanhou downstream pipeline, are you mainly meaning the NWB and Curriculum?

For NWB, we don't have the Randomness field stored in the old version. I updated the NWB to add the Randomness to different versions of behavior json.

For Curriculum, if the Randomness is not detected as a GUI field, will an error occur? Or just ignored.

hanhou commented 1 week ago

For Curriculum, if the Randomness is not detected as a GUI field, will an error occur?

Looks like we're fine in this case https://github.com/AllenNeuralDynamics/dynamic-foraging-task/blob/2fea58cc170beb75d6d92e31a9d7253e26c6bf57/src/foraging_gui/Dialogs.py#L2989

But please make sure that both are set to exponential by default for maximum backward compatibility.

XX-Yin commented 1 week ago

For Curriculum, if the Randomness is not detected as a GUI field, will an error occur?

Looks like we're fine in this case

https://github.com/AllenNeuralDynamics/dynamic-foraging-task/blob/2fea58cc170beb75d6d92e31a9d7253e26c6bf57/src/foraging_gui/Dialogs.py#L2989

But please make sure that both are set to exponential by default for maximum backward compatibility.

@hanhou I think the distribution of block length in the uncoupled task was hardcoded to uniform.

I will set the block distribution for uncoupled task to uniform, and for coupled task to exponential by default. And set the ITI and delay distribution to exponential by default.

XX-Yin commented 1 week ago

@hanhou We can hold off this pull request until the curriculum is updated. Setting default block lengths for different tasks would complicate the logic and be easy to make mistakes.

hanhou commented 1 week ago

I think the distribution of block length in the uncoupled task was hardcoded to uniform.

Oh, yes. But I still think we'll never use exponential block length in uncoupled task since it will further complicate the distribution of the "effective block length" in the uncoupled task. @ZhixiaoSu what do you think?