Skip to content

BF: starting and stopping times for EyetrackerRecordComponent#6607

Merged
TEParsons merged 5 commits intopsychopy:devfrom
mh105:dev-etrecord-test
Jun 24, 2024
Merged

BF: starting and stopping times for EyetrackerRecordComponent#6607
TEParsons merged 5 commits intopsychopy:devfrom
mh105:dev-etrecord-test

Conversation

@mh105
Copy link
Copy Markdown
Contributor

@mh105 mh105 commented Jun 22, 2024

  1. I tested out the change in BF: Add default duration to eyetracker record component #6599, and it doesn't impact experiment flow as far as my tests went. I'm proposing to revert it for clarify. @RebeccaHirst Perhaps you could provide a minimal test for the experiment halting behavior you described?
  2. I also looked into how non-slip timing behaves with EyetrackerRecordComponent, and I think it isn't behaving intuitively at the moment and can be buggy under certain situations. The BF here provides a simple solution.

A key ambiguity is what should determine the display of an EyetrackerRecordComponent on the timeline: should it show when the component starts and stops, or should it show when the underlying eyetracker recording starts and stops? Given our discussion in #6567, I think it makes more sense to decouple the concept of a component, which is what users are manipulating in Builder, from whatever underlying device (Sound, Eyetracker, etc.) is getting controlled by the component. This is especially important for EyetrackerRecordComponent because we have the unusual need of only starting or stopping an eyetracker.

I believe this agrees with the design principle behind how the start and stop times of a Component are used to determine whether a Routine is safe for non-slip timing. Right now if a user adds a Start Only type EyetrackerRecordComponent into a Routine, that will automatically make the routine not non-slip safe. This is not actually accurate because the EyetrackerRecordComponent is marked as FINISHED immediately after it starts, so it does not cause any additional timing uncertainty beyond what the rest of the routine is doing.

The visual display of the EyetrackerRecordComponent also runs beyond the right edge of the timeline. This is potentially confusing because the component itself is marked as FINISHED as soon as recording has started (updated in 9157901). Similarly for a Stop Only type EyetrackerRecordComponent, the visual display goes beyond the left edge of the timeline, but component is really just marked as STARTED on the first frame loop, not before. So if users are using the status of these Start Only and Stop Only type EyetrackerRecordComponents based on the visual timeline to control other parts of the experiment, there could be surprising behaviors.

Additionally, right now duration (s) is one of the options for stopping of a Stop Only type EyetrackerRecordComponent, but since the Start parameter field is hidden away, this component can't be set properly since there is missing start time to determine total duration.

Lastly, when we toggle among the three types of EyetrackerRecordComponent, while [startVal] or [stopVal] gets reset, [startType] or [stopType] doesn't. So if some start or stop type (such as Condition) was first used, and the parameter field got hidden away because users decided to employ a different actionType, that hidden parameter will still be set to the previous type behind the scene. This may cause strange behaviors difficult to debug.


A Component simply can't do all of 1) displaying the starting/stopping status of the underlying device, 2) displaying the status of the component itself, and 3) be used to determine non-slip timing, at the same time. We can only achieve two out of three. A simple way to break the current ambiguity is to intercept the getStartAndDuration() function call for EyetrackerRecordComponent. Depending on the actionType, we enforce any hidden parameter field to some standard values that should reflect the status of the Component (not the device) when displaying on the timeline, as well as that can be used to determine accurate timing, achieving 2) and 3).

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 50.62%. Comparing base (26f862d) to head (f67485e).
Report is 46 commits behind head on dev.

Current head f67485e differs from pull request most recent head 4375b1a

Please upload reports for the commit 4375b1a to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##              dev    #6607       +/-   ##
===========================================
+ Coverage   40.26%   50.62%   +10.35%     
===========================================
  Files         332      332               
  Lines       61048    61121       +73     
===========================================
+ Hits        24582    30941     +6359     
+ Misses      36466    30180     -6286     
Components Coverage Δ
app ∅ <ø> (∅)
boilerplate ∅ <ø> (∅)
library ∅ <ø> (∅)
vm-safe library ∅ <ø> (∅)

@TEParsons
Copy link
Copy Markdown
Contributor

Right now if a user adds a Start Only type EyetrackerRecordComponent into a Routine, that will automatically make the routine not non-slip safe. This is not actually accurate because the EyetrackerRecordComponent is marked as FINISHED immediately after it starts, so it does not cause any additional timing uncertainty beyond what the rest of the routine is doing.

This is a good point, and taken with the rest of your arguments I think makes a compelling case for eyetracker record objects to not be Components at all. Given that they don't necessarily start and stop in the same Routine, and instead carry out one single function and then move on, maybe they'd make more sense as a pair of StandaloneRoutines? @RebeccaHirst and I floated the idea when we were coming up with the current functionality, and I'm starting to think maybe we should have gone with it... I'll create an issue for further discussion.

For this specific fix, I think overloading getStartAndDuration is a good way to patch it up, but I'm unsure about changing the values of the params as it means something which should just access values (a get function) is actually changing them. Instead I think we should pass a copy of self.params to getStartAndDuration so we can change it as we need. Will put in a further commit, shouldn't be too much change :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐞 bug Issue describes a bug (crash or error) or undefined behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants