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

not able to compute event property duration #781

Closed
4 tasks done
SiQube opened this issue Aug 20, 2024 · 7 comments
Closed
4 tasks done

not able to compute event property duration #781

SiQube opened this issue Aug 20, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@SiQube
Copy link
Member

SiQube commented Aug 20, 2024

Current Behavior

when trying to calculate event duration as a property it fails.

Expected Behavior

it succeeds

or

remove it as event duration property

Failure Information (for bugs)

Steps to Reproduce

To reproduce:

import polars as pl
import pymovements as pm
import matplotlib.pyplot as plt

dataset = pm.Dataset('ToyDataset', path='data/toy')
dataset.download()
dataset.load()
dataset.pix2deg()
dataset.pos2vel('smooth')
dataset.detect_events('ivt')
dataset.compute_event_properties('duration')

Context

Please provide any relevant information about your setup.
This is important in case the issue is not reproducible except for under certain conditions.

  • Project Version / Commit: stable
  • Operating System: Ubuntu 24.04

Failure Logs

(venv) c-cube@c-cube-VirtualBox:/tmp/pymovements$ python s.py 
Downloading http://github.com/aeye-lab/pymovements-toy-dataset/zipball/6cb5d663317bf418cec0c9abe1dde5085a8a8ebd/ to data/toy/downloads/pymovements-toy-dataset.zip
pymovements-toy-dataset.zip: 100%|████████████████████████████████████████████████████| 3.06M/3.06M [00:01<00:00, 2.99MB/s]
Checking integrity of pymovements-toy-dataset.zip
Extracting pymovements-toy-dataset.zip to data/toy/raw
100%|██████████████████████████████████████████████████████████████████████████████████████| 20/20 [00:00<00:00, 61.64it/s]
100%|██████████████████████████████████████████████████████████████████████████████████████| 20/20 [00:00<00:00, 36.65it/s]
100%|██████████████████████████████████████████████████████████████████████████████████████| 20/20 [00:00<00:00, 47.69it/s]
20it [00:00, 71.12it/s]
0it [00:00, ?it/s]
Traceback (most recent call last):
  File "/tmp/pymovements/s.py", line 11, in <module>
    dataset.compute_event_properties('duration')
  File "/tmp/pymovements/src/pymovements/dataset/dataset.py", line 640, in compute_event_properties
    new_properties = processor.process(
                     ^^^^^^^^^^^^^^^^^^
  File "/tmp/pymovements/src/pymovements/events/processing.py", line 209, in process
    values = filtered_gaze.select(
             ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/pymovements/venv/lib/python3.12/site-packages/polars/dataframe/frame.py", line 8008, in select
    return self.lazy().select(*exprs, **named_exprs).collect(_eager=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/pymovements/venv/lib/python3.12/site-packages/polars/lazyframe/frame.py", line 1706, in collect
    return wrap_df(ldf.collect())
                   ^^^^^^^^^^^^^
polars.exceptions.ColumnNotFoundError: offset

Error originated just after this operation:
DF ["time", "stimuli_x", "stimuli_y", "text_id"]; PROJECT */8 COLUMNS; SELECTION: "None"

Checklist

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed
  • I have provided sufficient information for the team
@SiQube SiQube added the bug Something isn't working label Aug 20, 2024
@SiQube
Copy link
Member Author

SiQube commented Aug 20, 2024

IMO, I'd remove it as a event property because we already automatically calculate it, see: here and I don't see a case where we want to recompute it.

@saeub
Copy link
Collaborator

saeub commented Aug 26, 2024

@SiQube What about datasets that contain event data? Duration might not be included in those, no?

@SiQube
Copy link
Member Author

SiQube commented Aug 26, 2024

if we just have event data, if the gaze data is not available and we have not given durations. how can we compute the duration?

(event) duration property necessarily needs (raw) gaze data.

@SiQube
Copy link
Member Author

SiQube commented Aug 26, 2024

another solution would be to have two cases in the event(property)processor, one for duration and one for all other cases (see here)

@SiQube
Copy link
Member Author

SiQube commented Aug 26, 2024

@SiQube What about datasets that contain event data? Duration might not be included in those, no?

Currently calculating event properties is only supported for datasets containing gaze dataframes. since #716 is merged I am planning to add them to precomputed event dataframes as well but will take me a while

@SiQube
Copy link
Member Author

SiQube commented Mar 14, 2025

@dkrako @saeub see #1037

@dkrako
Copy link
Contributor

dkrako commented Mar 14, 2025

This issue is very confusing due to all the erroneous assumptions made.

@saeub is right in that we need the duration property to calculate duration for loaded event files without a duration column.

IMO, I'd remove it as a event property because we already automatically calculate it, see: here and I don't see a case where we want to recompute it.

There's at least two use cases to recompute duration: (1) duration is not given in milliseconds. (2) there are calculation errors in the duration columns.

if we just have event data, if the gaze data is not available and we have not given durations. how can we compute the duration?
(event) duration property necessarily needs (raw) gaze data.

The duration property does not need any event data at all. It only needs the onset and offset columns in the event data frame.

another solution would be to have two cases in the event(property)processor, one for duration and one for all other cases (see here)

There's no need in adjusting the EventGazeProcessor as duration can be computed with EventProcessor which doesn't need any gaze data frames, just event event data frames.

Currently calculating event properties is only supported for datasets containing gaze dataframes. since #716 is merged I am planning to add them to precomputed event dataframes as well but will take me a while

All the precomputed stuff should be deprecated as soon as possible before anyone really starts using this.

As this issue will only confuse other contributors an the forwarded proposals do not address any of the actual underlying issues, I close this in favor of these:

@dkrako dkrako closed this as completed Mar 14, 2025
@dkrako dkrako reopened this Mar 14, 2025
@dkrako dkrako closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants