Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding second stimulus #1276

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from
Open

Adding second stimulus #1276

wants to merge 33 commits into from

Conversation

XX-Yin
Copy link
Collaborator

@XX-Yin XX-Yin commented Jan 5, 2025

Pull Request instructions:

  • Please follow the update protocol
  • Answer the questions below in detail. Your responses will be emailed to experimenters.
  • If the experimenters must do anything new, provide detailed step by step instructions on the wiki
  • If computer maintainers need to manually update anything, provide detailed step by step instructions
  • Use markdown syntax in order for your comments to be rendered reliably in the email: "1." instead of "1)", use four spaces for indents.
  • If you use the keyword "skip email" in the title, it will skip the email updates
  • Merges from "develop" into "production_testing" should use the keyword "production merge" in the title for reliable indexing of updates
  • Merges from "production_testing" into "main" should use the keyword "update main"

Describe changes:

  1. Add a second auditory stimulus.
  2. Differentiate the auditory cues for rewarded and non-rewarded trials. The rewarded auditory cue is stored at index 5 of the sound card, while the non-rewarded auditory cue is stored at index 6. If there is no response, no sound will be played.
  3. Introduce a delay to control the timing between the reward outcome (reward or no reward) and the second cue.
  4. Replace the fixed reward delay with a distribution (exponential or uniform). If the second stimulus is enabled, the reward delay corresponds to the second "go" cue. Otherwise, it corresponds to the reward outcome.
    image

What issues or discussions does this update address?

#1263 (comment)

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

This update aligns with our current pipeline, but a few adjustments are necessary:

  • Adding the following variables to the NWB file:

    • B_SecondStimulusDelay: The second stimulus delay sampled for each trial.
    • B_GoCueTimeSoundCardSecondStimulus: The harp timestamp of the second sound.
    • B_RewardDelay: The reward delay time sampled for each trial.
    • TP_GiveSecondStimulus: A parameter indicating whether the GiveSecondStimulus is turned on or off.
    • TP_RandomnessSecondStimulus: A parameter defining the random sampling method used for the second stimulus delay.
    • TP_SecondStimulusBeta: A parameter specifying the beta value used for the exponential distribution of the second stimulus delay (not applicable for uniform distribution).
    • TP_SecondStimulusMin: A parameter specifying the minimum value for the second stimulus delay.
    • TP_SecondStimulusMax: A parameter specifying the maximum value for the second stimulus delay.
    • TP_RandomnessRewardDelay: A parameter defining the random sampling method used for the reward delay.
    • TP_RewardDelayBeta: A parameter specifying the beta value used for the exponential distribution of the reward delay (not applicable for uniform distribution).
    • TP_RewardDelayMin: A parameter specifying the minimum value for the reward delay.
    • TP_RewardDelayMax: A parameter specifying the maximum value for the reward delay.
  • Deleting the following variables to the NWB file:

    • TP_RewardDelay
  • Adding the following variables to the curriculum

    • GiveSecondStimulus: Default is off if not set from the curriculum
    • RandomnessSecondStimulus: Default is Even if not set from the curriculum
    • SecondStimulusBeta: Default is 2
    • SecondStimulusMin: Default is 1
    • SecondStimulusMax: Default is 8
    • RandomnessRewardDelay: Default is Even
    • RewardDelayBeta: Default is 2
    • RewardDelayMin: Default is 1
    • RewardDelayMax: Default is 8
  • Deleting the following variables from the curriculum

    • RewardDelay
  • Uploading waveform to index 5 and 6 of the soundcard to use the second stimulus.

Describe any manual update steps for task computers

Was this update tested in 446/447?

This PR is a prototype for adding a second stimulus, and it will be handled over to @rachelstephlee @hagikent for full testing, updates and modifications. @alexpiet @hanhou @bruno-f-cruz @micahwoodard for sanity check.

@XX-Yin XX-Yin changed the title Ading second stimulus Adding second stimulus Jan 6, 2025
@hagikent
Copy link
Collaborator

hagikent commented Jan 6, 2025

Great! Thanks @XX-Yin
I will check the changes in the bonsai side later today. I cannot critically read GUI side, so better letting @micahwoodard

Uploading waveform to index 5 and 6 of the soundcard to use the second stimulus.

I will take care of this too.

@bruno-f-cruz
Copy link
Collaborator

bruno-f-cruz commented Jan 7, 2025

Here's the refactored version (#1286).
I focused on the snippet you sent above without changing anything else. So I hope I understood the scope of your changes.
It fixes a likely bug you might have encountered when testing where the SelectMany would not close on each OnNext event (try to close the inner sequence, if you want to be explicit about it, just use a simple Take(1) in most cases). If I am understanding the logic correctly, I think the board would still play the sound but you would progressively send more and more messages per trial. This would likely increase the playback of the sound by 1-2ms per trial which could become a problem at some point. It also refactors a bit of the logic to make it easier to maintain in the future should you need to. If you have problems let me know.

I have also refactored the workflow to make it easier to read and maintain in the future should you require. In general I advise abstracting the functionality as much as possible from the hardware (e.g. Coding the tones via subjects, and converting the logic into hardware instructions., i.e. HarpMessages, as late as possible). This makes code more readable for anyone trying to maintain it but also easier to refactor in the future should you need to use something else to deliver a tone.

image

If you find bugs or have questions, lmk!

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 7, 2025

@bruno-f-cruz Thank you! This looks great! @hagikent and @rachelstephlee will test your updates.

I think you're likely right that "SelectMany would not close", but during testing, I didn't observe more and more messages. Could you show me sometime? I'm really curious about this.

@rachelstephlee
Copy link
Contributor

thank you everyone for getting this done so fast! our meeting on thursday will end early if we dont' need to do much (i hate long meetings).

see you in a few days!

@rachelstephlee
Copy link
Contributor

In preparation for Thursday, Kenta and I will try to figure out the starting parameters we will want to use for this set up. Please let us know if there's anything else we can prepare ahead of time for our Thursday meeting so we can have a quick and efficient meeting.

So far, the parameters we will need to provide are:

  • tone sounds for CS+/CS-
  • timing between key events (go cue -> lick choice -> CS+/CS- -> new trial etc).

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 8, 2025

a little more detail about parameters in the GUI if you plan to test it. Related parameters are shown below. And the second stimulus should be selected as on.

image

@hagikent
Copy link
Collaborator

hagikent commented Jan 8, 2025

@XX-Yin
Thanks! I suppose beta/min/max are for exponential dist? Is the distribution generated on the GUI side or Bonsai-side?
@bruno-f-cruz the other day showed me how to flexibly generate different distribution in Bonsai including constant values. Can we adopt it?

We can discuss this tomorrow, thanks!

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 8, 2025

@hagikent
The distribution is controlled by the randomness parameter. If it's selected as the Exponential, beta is involved. If it's selected as even, then only use min and max will be used. Currently, the randomness parameter also controls the random distribution of the ITI and delay. You can add an independent randomness control for the second stimulus if you want.

The distribution is generated on the GUI side.

@hagikent
Copy link
Collaborator

hagikent commented Jan 9, 2025

thanks for the clarification @XX-Yin !
in a long-run, we might want to let bonsai to take care of rand-get. let's discuss tomorrow.

Copy link
Collaborator

@hagikent hagikent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment on dist selection.

Also make the default parameters for the second delay/reward delay more realistic!
(credit assignment won't work with ~8s reward delay!)

@@ -392,16 +395,23 @@ def _generate_next_trial_other_paras(self):
# get the ITI time and delay time
if self.TP_Randomness=='Exponential':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make distributions independently controlable.
-ITI needs to be exp so that it gives near-flat hazard.
-@rachelstephlee prefers normal dist for CS/Reward delays

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 9, 2025

I didn't see repeated messages, there were 5 trials with 5 messages to the soundcard. However, I added take 1 to the SelectMany for extra safety.

image

@hagikent
Copy link
Collaborator

hagikent commented Jan 9, 2025

ah, @XX-Yin are you directly modifying this branch?
I thought, first merging @bruno-f-cruz 's bc branch into yours, and then adding minor fix would be the strategy.

I'm ok if the former is easier for you! Just for clarification.

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 9, 2025

ah, @XX-Yin are you directly modifying this branch? I thought, first merging @bruno-f-cruz 's bc branch into yours, and then adding minor fix would be the strategy.

I'm ok if the former is easier for you! Just for clarification.

I think we can go with the current branch without merging Bruno's branch.

@hagikent
Copy link
Collaborator

hagikent commented Jan 9, 2025

Ok, sounds good.
Hope @micahwoodard can review the modification in the GUI side, in particular, newly added parameter fields are correct.

@XX-Yin
Also, you mentioned we need to update nwb packaging, correct? Can you unpack it a bit? The pipeline needs upgrade?

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 9, 2025

@hagikent
I will give a detailed description about changes we need to make on the NWB packaging and the Curriculum after adding two extra parameters to independently control the distribution of the second delay and the reward delay.

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Jan 13, 2025

@hagikent @rachelstephlee I added the independent randomness control to the second stimulus and the reward delay. I also described the updates required for the downstream pipeline in the pull request comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants