-
Notifications
You must be signed in to change notification settings - Fork 38
feat: add load_text_stimuli()
to Dataset
#1267
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
base: main
Are you sure you want to change the base?
Conversation
- fixed the test for correct text stimulus loading - changed mock_toy function and added a ToyAOI value to the fixture
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
===========================================
- Coverage 100.00% 99.97% -0.03%
===========================================
Files 104 104
Lines 4512 4554 +42
Branches 783 788 +5
===========================================
+ Hits 4512 4553 +41
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shorten example aoi file
deleted unnecessary aoi file
…com/aeye-lab/pymovements into feature/add-load-stimuli-in-dataset
…com/aeye-lab/pymovements into feature/add-load-stimuli-in-dataset
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
load_text_stimuli()
to Dataset
load_text_stimuli()
to Dataset
load_text_stimuli()
to Dataset
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.
really cool feature, thank you for working on it! some minor feedback (already)
@pytest.mark.parametrize( | ||
'expected', | ||
[ | ||
EXPECTED_AOI_MULTIPLEYE_STIMULI_TOY_X_1_TEXT_1_1, |
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.
please add an additional test using the existing aoi file in tests/files
def test_stimuli_list_exists(gaze_dataset_configuration): | ||
dataset = Dataset(**gaze_dataset_configuration['init_kwargs']) | ||
|
||
assert isinstance(dataset.stimuli, list) |
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.
this could also be an empty list
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.
This is already looking really great! Apart from adding some tests for the existing AOI files, I would probably revert the hard removal of the from_file()
to avoid a breaking change for now. I deprecate this in a follow-up PR.
@@ -71,6 +71,7 @@ def __init__( | |||
self.events: list[Events] = [] | |||
self.precomputed_events: list[PrecomputedEventDataFrame] = [] | |||
self.precomputed_reading_measures: list[ReadingMeasures] = [] | |||
self.stimuli: list[TextStimulus] = [] |
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 this is fine for this PR, but we should probably create a more general Stimulus
base class. Moreover, we should think about Stimulus
collections, as an individual Stimulus
may be mapped to some trials in the experiment. I will create an issue regarding these.
@@ -131,6 +133,8 @@ def load( | |||
:py:meth:`pymovements.Dataset.path`. | |||
This argument is used only for this single call and does not alter | |||
:py:meth:`pymovements.Dataset.preprocessed_rootpath`. (default: None) | |||
stimuli_dirname: str | None | |||
:py:meth:`pymovements.Dataset.stimuli_rootpath`. (default: 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.
probably missed to paste
One-time usage of an alternative directory name to save data relative to
:py:meth:`pymovements.Dataset.path`.
This argument is used only for this single call and does not alter
if self.definition.resources.has_content('stimuli'): | ||
self.load_text_stimuli() | ||
# stimuli_dirname=stimuli_dirname, # TODO custom dir name | ||
# extension=extension, |
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.
passing the extension won't be necessary, as it's more related to the gaze files.
@@ -203,6 +203,28 @@ class DatasetDefinition: | |||
transformations. If not specified, the constant eye-to-screen distance will be taken from | |||
the experiment definition. This column will be renamed to ``distance``. (default: None) | |||
|
|||
aoi_content_column: str | 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.
For this PR I would leave these fields at this level, but they should probably be moved to the ResourceDefinition
level before the next release. I need to think about how to integrate these best without crowding the ResourceDefinition
with lots of fields.
Maybe we should create classes like StimulusResourceDefinition
, SamplesResourceDefinition
, EventsResourceDefinition
, LabelsResourceDefinition
. These would then inherit from ResourceDefinition
but include the more specific fields associated with these content types.
Alternatively, #1270 paved the way for having load_kwargs
in the DatasetDefinition
. Maybe we could use these.
I'll need to think about this a bit more and write up an issue.
|
||
def from_file( |
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 would probably like to deprecate this function instead of removing it to prevent a breaking change.
But I'll do this in a separate PR after merging this. For your just revert the removal and reuse TextStimulus.from_file()
in the old function.
Description
Implemented loading text stimuli in the Dataset.load() method.
Implemented changes
Additions:
How Has This Been Tested?
All the previously implemented tests passed
Added a dataset_type "ToyAOI", which includes a
stimuli
folder with files imported from tests/files/aoi_multipleye_stimuli_toy_x_1 (not generated in contrast to the other dataset_types)We added a test for
Type of change
Context
Partially resolves
Dataset
#1233related issues:
Checklist:
Future work and comment on warnings