-
Notifications
You must be signed in to change notification settings - Fork 34
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
VEP calibration #304
VEP calibration #304
Conversation
…andling for symbols prior to putting them in boxes; updated VEP calib demo
This reverts commit 437572ace09eb107aa09966e5a8360673131ea4d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge amount of effort here! I added some comments. The configurability one is the only thing I am requesting changes on before accepting.
It's ready to time test, I think? Please schedule a time to test it in the lab with me! I think we can use this with a photodiode as-is, but happy to wear the cap if that falls through.
bcipy/parameters/parameters.json
Outdated
"recommended_values": "", | ||
"type": "float" | ||
}, | ||
"vep_flicker_rates": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should support this kind of parameter configuration... We could do a range or discuss a different configuration strategy (task params). If this isn't a parameter we need to configure for an experiment, we can also set it ourselves. I'd support a code default and a separate PR to address configurability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll do some refactoring here. I was mainly concerned about having consistent, known values between the calibration and a future copy phrase. I wasn't sure what values to use so I started with ones close the the Higger paper. I'm fine putting these elsewhere if we don't want them easily changeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to add List of Int or Float support to our parameters. Perhaps we can check-in with the team during demo to see what the short-term experiment needs are?
# get the number of frames per flicker | ||
length_flicker = refresh_rate / flicker_rate | ||
|
||
if length_flicker.is_integer(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure you added this, so my apologies, but if this checks the type already, we could potentially do a if not length_flicker.is_integer()
and avoid the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast is needed below (line 62) for list generation: codes += [t] * length_flicker
. If it's missing you get a TypeError that you can't multiply a sequence by a float.
if length_flicker.is_integer(): | ||
length_flicker = int(length_flicker) | ||
else: | ||
err_message = f'flicker rate={flicker_rate} is not an integer multiple of refresh rate={refresh_rate}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could put the calculated value here too
bcipy/display/paradigm/vep/layout.py
Outdated
positions += [(layout.horizontal_middle, top), | ||
(layout.horizontal_middle, bottom)] | ||
return positions | ||
if self.num_boxes == 4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might consider making these a type (4by4VEP, etc) to help set other properties as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this context the num_boxes refers to the number of display areas where a symbol can be sorted. The VEPStim is where the number of squares per checkerboard is defined. If we only ever want to fix this at 6 display areas, I can remove the conditional logic. The other idea here is that if we want a different layout we can provide another class with the same interface for generating these positions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that last idea! Unless we are going to test 4 vs 6 in the short term, I think it would be easier to maintain cutting it out.
|
||
# Checkerboard is represented as a polygon with holes, backed by a | ||
# simple square with the alternating color. | ||
# This technique renders more efficiently and scales better than using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good to know. The only other VEP I could find used the GradientStim. Is the performance gain due to the decrease in objects drawn overall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried various stim for this and there wasn't a significant difference. The main factor was the number of stim drawn.
""" | ||
if target == symbol: | ||
return TriggerType.TARGET | ||
return TriggerType.EVENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to support different TriggerTypes for this. A good question for the group at demo. What triggers will be needed for reshaping / future analysis. I added a few comments in the display that I know we will want.
def __init__(self, win: visual.Window, daq: ClientManager, | ||
parameters: Parameters, file_save: str): | ||
super(VEPCalibrationTask, self).__init__() | ||
self.num_boxes = 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could deprecate 4x4 until it is used if we are hardcoding. I'm thinking about long term maintainability and it would prevent some branching logic.
|
||
|
||
def init_calibration_display(parameters: Parameters, | ||
window, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing
timing(list[list[float]]): list of timings | ||
color(list(list[str])): list of colors)): scheduled inquiries | ||
""" | ||
if timing is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be okay to fail without these instead of making optional.
|
||
def generate_vep_inquiries(symbols: List[str], | ||
num_boxes: int = 6, | ||
inquiry_count: int = 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here on defaults, it would make sense to have the caller define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task runs as expected! Left a few cleanup items. This still needs to be tested more fully in the lab, but I'd be okay merging with some clean up and making follow-up VEP PRs to the rc or closing this and making future VEP PRs to this branch.
|
||
log = logging.getLogger(__name__) | ||
|
||
DEFAULT_FLICKER_RATES = [4, 5, 6, 10, 12, 15] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having tested this now in lab, having these calculated or a default dictionary for different Hz would be helpful! This could live in config.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more details for this to https://www.pivotaltracker.com/story/show/186641183.
stim_color=colors, | ||
inquiry=[], | ||
stim_length=1, | ||
animation_seconds=1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull from parameters?
height=parameters['task_height']) | ||
|
||
# issue #186641183 ; determine a better configuration strategy | ||
flicker_rates = DEFAULT_FLICKER_RATES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log these
random_target_positions) | ||
|
||
|
||
def generate_vep_calibration_inquiries(alp: List[str], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could go in helpers with the others. I'd be okay breaking our old pattern but wanna be consistent or make some tickets to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stimuli module is getting pretty large and these functions seemed specific to this task. I also think they are more likely to change, so it made sense to me to split these out. I think it would be good to revisit the stimuli
module and see if some of the things in there would make sense elsewhere. I'll create a ticket for that.
…than codes; added logging
Overview
Created a new calibration task using the VEPDisplay.
Ticket
https://www.pivotaltracker.com/story/show/184564388
https://www.pivotaltracker.com/story/show/186126657
Contributions
Test
Discussion
For testing this locally, the following timing parameters worked well.
time_prompt: 4
time_fixation: 0.5
time_flash: 4
I created two followup issues:
https://www.pivotaltracker.com/story/show/186522989
https://www.pivotaltracker.com/story/show/186522657