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

Calib all symbols #287

Merged
merged 8 commits into from
Aug 1, 2023
Merged

Calib all symbols #287

merged 8 commits into from
Aug 1, 2023

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Jul 28, 2023

Overview

Refactored stimuli generation to ensure that all symbols are shown in a calibration.

Ticket

https://www.pivotaltracker.com/story/show/185235958

Contributions

  • Refactored stimuli generation to be break large functions into smaller units that can be more easily tested and combined.
  • Added new criteria for targets (frequency)
  • Added test coverage
  • Added utility functions for analyzing the statistical properties of symbols in an inquiry schedule.

Test

  • Ran unit tests, including those to ensure that previous criteria are still being met
  • Ran a sample calibration to ensure the refactored names were properly integrated in the tasks.

lawhead added 7 commits July 10, 2023 17:36
…istribution. Factored out calculation of random_target_positions into its own function which could be thoroughly tested. Added unit tests for new function
…ries. Refactored to ensure that symbols were presented as targets with equal frequency
@lawhead lawhead requested review from tab-cmd and celikbasak July 28, 2023 17:59
Copy link
Contributor

@tab-cmd tab-cmd left a comment

Choose a reason for hiding this comment

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

This is a really clean addition! Thanks for the PR, Matt!

target_indexes = distributed_target_positions(stim_number, stim_length, nontarget_inquiries)
if timing is None:
timing = [0.5, 1, 0.2]
if color is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be okay making these required inputs. I don't think we gain much from having these defaults.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the main advantage is in testing and demos when we primarily care about the symbols.

stim_order: StimuliOrder = StimuliOrder.RANDOM,
target_positions: TargetPositions = TargetPositions.RANDOM,
nontarget_inquiries: int = 10,
percentage_without_target: int = 10,
Copy link
Member

Choose a reason for hiding this comment

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

Based on our previous discussions maybe we can scrap this number and set the default as 0?

Copy link
Member

@celikbasak celikbasak left a comment

Choose a reason for hiding this comment

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

This is a great update, thank you so much Matt!

@lawhead lawhead merged commit aca61e9 into 2.0.0rc4 Aug 1, 2023
@lawhead lawhead deleted the calib-all-symbols branch August 30, 2023 20:25
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.

3 participants