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

Output stim positions, capture screenshots, more cleanup #303

Merged
merged 10 commits into from
Dec 5, 2023

Conversation

tab-cmd
Copy link
Contributor

@tab-cmd tab-cmd commented Nov 20, 2023

Overview

Add functionality to output stimuli positions and capture a screenshot. The output also contains the screen size, and can be used to further adjust visualizations for greater accuracy (currently unused).

In addition, further typing and cleanup.

Ticket

Contributions

  • bcipy/config.py: add new matrix options
  • visualization.py: use new config path
  • offline_analysis.py: use new config path, extract from the session folder, and fix argument passing to visualization.
  • save.py: add a stimuli position export method
  • task.py: add export of positions, cleanup on tasks, remove unneeded abc method on task, typing, add calibration tests
  • feedback.py: deprecate level feedback (no tests and unused). We could potentially deprecate the Audio feedback for now as well...
  • matrix/display.py add capture screenshot method
  • .coveragerc: ignore the exceptions
  • requirements.txt: upgraded to lock PyQt6 dependancies failing Windows builds. See PyQt6: DLL load failed while importing QtGui: The specified procedure could not be found. altendky/pyqt-tools#105

Test

  • make test-all
  • bcipy --fake --task "Matrix Calibration" and ensured the output looked correct

Documentation

  • Are documentation updates required? In-line, README, or documentation? in-line and task/README.md updated.

Changelog

  • Is the CHANGELOG.md updated with your detailed changes? TODO!

time = client.offset(
self.matrix.first_stim_time) - self.matrix.first_stim_time
triggers.append(Trigger(label, TriggerType.OFFSET, time))
self.trigger_handler.add_triggers(triggers)

# make sure triggers are written for the inquiry
self.trigger_handler.add_triggers(convert_timing_triggers(timing, timing[0][0], self.trigger_type))

def write_offset_trigger(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to match the recent update to support multiple device offsets.. I think this is correct @lawhead

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I just noticed this missing. Looks right to me.

@@ -23,7 +22,6 @@
from bcipy.task.paradigm.rsvp.copy_phrase import RSVPCopyPhraseTask


@pytest.mark.slow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to help me on the typing PR. It was merged by accident!

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.

Great additions! Thank you for including the calibration tests!
For symbol positions, maybe we can introduce a parameter/saving option in helpers/save.py for saving the units in Tobii for ease of use.

grid[sym] = visual.TextStim(win=self.window,
text=sym,
color=self.grid_color,
opacity=self.start_opacity,
pos=self.positions[pos_index],
pos=pos,
Copy link
Member

Choose a reason for hiding this comment

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

Is this the position in psychopy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the visual.TextStim is a psychopy display component, so the position information is expected to be a valid psychopy format, depending on the units. When no units are provided it uses the default 'norm' units.

screen_info: dict) -> str:
"""Save stimuli positions and screen info to `path`

stimuli_position_info: {'A': (0, 0)}
Copy link
Member

Choose a reason for hiding this comment

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

Same question here, I think it would be more informative if we mentioned the units here (pyschopy, Tobii or maybe just the screen limits like (0,0), (1,1), (-1,-1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'm happy to put those in the export as well. We will need to to some testing to see how these line up. I should have data to look at by EOD today.

show=show_figures,
raw_plot=True,
raw_plot=plot_points,
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

"""
# draw the grid and flip the window
self.draw_grid(opacity=self.full_grid_opacity)
self.task_bar.current_index = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would't expect this method to mutate the object state. It seems like this has the potential to lead to some unexpected bugs. Maybe have a temp variable here and set it back to its previous value after the screenshot?

time = client.offset(
self.matrix.first_stim_time) - self.matrix.first_stim_time
triggers.append(Trigger(label, TriggerType.OFFSET, time))
self.trigger_handler.add_triggers(triggers)

# make sure triggers are written for the inquiry
self.trigger_handler.add_triggers(convert_timing_triggers(timing, timing[0][0], self.trigger_type))

def write_offset_trigger(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I just noticed this missing. Looks right to me.

assert 'screen_size_pixels' in screen_info.keys(), \
'screen_size_pixels must be a key in screen_info'
assert isinstance(screen_info['screen_size_pixels'], list), \
'screen_size_pixels must be a list'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the amount of input validation required would it make sense to have some stricter parameter types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true with the new typing it should be mostly caught... I will remove some of this (minus key:value assert) and avoid a more explicit data structure for now.

grid[sym] = visual.TextStim(win=self.window,
text=sym,
color=self.grid_color,
opacity=self.start_opacity,
pos=self.positions[pos_index],
pos=pos,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the visual.TextStim is a psychopy display component, so the position information is expected to be a valid psychopy format, depending on the units. When no units are provided it uses the default 'norm' units.

@tab-cmd tab-cmd merged commit 4102a83 into 2.0.0rc4 Dec 5, 2023
6 checks passed
@tab-cmd tab-cmd deleted the output_stim_positions branch December 5, 2023 01:51
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