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

Multimodal config #277

Merged
merged 5 commits into from
Apr 18, 2023
Merged

Multimodal config #277

merged 5 commits into from
Apr 18, 2023

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Apr 12, 2023

Overview

Added functionality for configuring multiple acquisition devices.

Ticket

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

Contributions

  • Added an acquisition ClientManager to manage multiple clients
  • Refactored acquisition helper to take an 'acq_mode` parameter and parse it to determine which devices to use. The mode can include just the content type ('EEG') or the content type and device name ('EEG/DSI-24'). Devices are separated by a '+' delimiter ('EEG+Eyetracker').
  • Refactored to always generate devices.json in the data folder.
  • Refactored 'analysis_channels' method to take a device_spec argument, and ensured that all uses of the method get the spec from the appropriate devices.json file.

Test

  • Ran unit tests and linting
  • Ran Calibration and Copy Phrase using a number of different scenarios:
    • calibration with fake data and acq_mode: 'EEG'
    • calibration with fake data and acq_mode 'EEG/DSI-VR300'
    • calibration with fake data and acq_mode: 'EEG+Eyetracker'
    • calibration with 'real' data and acq_mode: 'EEG'
    • offline analysis for full calibration session
    • copy phrase with real data and acq_mode: 'EEG'
    • copy phrase with real data and acq_mode: 'EEG+Eyetracker'

Discussion

This PR is for the configuration and recording of data for the configured acquisition devices. However, only the EEG device is currently passed to the tasks. It will be a future refinement to pass the client_manager to the tasks so the different devices can be queried.

@lawhead lawhead requested review from tab-cmd and celikbasak April 12, 2023 23:42
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 worked well in my testing locally! @celikbasak please coordinate testing this to make sure it works for you in the upcoming experiment



class ClientManager():
"""Manages multiple acquisition clients. This class can also
Copy link
Contributor

Choose a reason for hiding this comment

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

++ some documentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops! Looks like I deleted what I had written. Good catch.


"""

def __init__(self, default_content_type='EEG'):
Copy link
Contributor

Choose a reason for hiding this comment

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

type

"""Retrieve the DeviceSpec with the given name. An exception is raised
if the device is not found."""
device = preconfigured_devices().get(name, None)
if not device:
if strict and not device:
raise ValueError(f"Device not found: {name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

We could give a more direct pointer here to our spec or current support devices

@tab-cmd
Copy link
Contributor

tab-cmd commented Apr 15, 2023

++ CHANGELOG.md

Copy link
Member

Choose a reason for hiding this comment

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

This Copy Phrase Demo does not do any updates using the eye gaze data right? In other words, I see the the preconfigured device is DSI-24 here, but even if we updated that and collected multimodal data the decision maker would solely rely on the EEG data, is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct. The eye tracker data would be collected but not yet used for any decision making. At this point only the EEG device is sent to the tasks. We created another ticket for how to integrate the additional devices in the tasks. This will take a bit of discussion.

return device


def with_content_type(content_type: str) -> List[DeviceSpec]:
Copy link
Member

Choose a reason for hiding this comment

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

This is a great addition. Using content type (Eyetracker) instead of device name (Tobii-P0) can solve the problem we encountered here https://www.pivotaltracker.com/story/show/184882983.

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 huge! Thank you so much Matt! I will test multimodal data acquisition within this week.

@lawhead lawhead merged commit 893c509 into 2.0.0rc3 Apr 18, 2023
@lawhead lawhead deleted the multimodal-config branch April 18, 2023 17:10
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