WIP: Calibration Interrupt by maddycapp27 · Pull Request #359 · LABSN/expyfun

maddycapp27 · 2018-07-10T15:05:17Z

I'd attempted this before and it was surprisingly messy so guidance/debate is welcome. @larsoner @drammock @rkmaddox

The goal is to be able to re-calibrate during an experiment when a given key is pressed.

Things to consider:

ec has the button press info but el needs to be called -- so which should the function live in? I'd previously gotten it to "work" with the function living in ec and importing el to ec after both were initialized but it was pretty gross.
Which functions (if any) should automatically check for re-calibration? We'd want to be able to re-calibrate before a trial if the subject isn't able to get fixation (and then probably abort the rest of the trial after calibration) and also between trials (so we could re-calibrate after a break).
Any key press functions in input controllers like _retrieve_keyboard_events or _correct_presses will need to recognize the calibration key.

maddycapp27 · 2018-07-10T15:06:48Z

@larsoner It looks like circle build is working

larsoner · 2018-07-10T17:35:32Z

Which functions (if any) should automatically check for re-calibration? We'd want to be able to re-calibrate before a trial if the subject isn't able to get fixation (and then probably abort the rest of the trial after calibration) and also between trials (so we could re-calibrate after a break).

I don't think any should, I think you should make a function to do this.

It should probably be a method of EyelinkController, because it already contains a reference to its parent ec, so it can call the proper ec polling function.

Any key press functions in input controllers like _retrieve_keyboard_events or _correct_presses will need to recognize the calibration key.

This sounds too messy, as it requires burdening generic press-event code with paradigm-specific checks. A cleaner design is probably to know that there is some interval during which a recalibration can be requested, and press the button then.

maddycapp27 · 2018-07-24T16:28:29Z

@rkmaddox and I were talking about how this might work in an experiment. Perhaps writing ec.call_on_every_wait() function might allow us to check for recalibration automatically without having to include several instances of el.check_calibration() in experiment code.

larsoner · 2018-07-24T16:29:20Z

Sounds reasonable to me

codecov-io · 2018-07-30T17:33:41Z

Codecov Report

Merging #359 into master will decrease coverage by 0.09%.
The diff coverage is 74.28%.

@@            Coverage Diff            @@
##           master     #359     +/-   ##
=========================================
- Coverage   89.07%   88.98%   -0.1%     
=========================================
  Files          44       44             
  Lines        5682     5717     +35     
  Branches      917      926      +9     
=========================================
+ Hits         5061     5087     +26     
- Misses        426      430      +4     
- Partials      195      200      +5

maddycapp27 · 2018-07-30T17:44:23Z

Have not tested this with an actual eyelink connected yet and have not added tests.

I've started with what I think the minimum scope of ec.on_every_wait() should be -- it's only included in wait_secs in _utils. To my knowledge this gets called for ec.wait_secs() and at the beginning of functions that have a min_wait parameter. The next thing is whether we want wait functions to be called by ec.screen_prompt(), ec.wait_one_press(), etc. The only other place I can think to include it would be during el.wait_for_fix() but doing so would call for aborting a trial and probably should be done on an experiment by experiment basis.

rkmaddox · 2018-07-30T17:55:48Z

My feeling is that putting the functions into the base waiting function (which I think is wait_secs ?) is the way to do it. That way every function that waits at all, including the ones you listed, calls the on_every_wait functions at some interval.

rkmaddox · 2018-07-30T18:01:54Z

Ahh, I see what you're saying. Functions like wait_one_press don't use wait_secs, so they'll also require modification. That's a tough one. I think those should also do the on_every_wait functions? Or maybe have a flag that defaults to True? Is there ever a reason we wouldn't want them to run?

maddycapp27 · 2018-07-30T18:02:27Z

Yes, wait_secs is called during those functions in _init_wait_click or _init_wait_press for the duration of min_wait but then the remainder of the waiting is controlled by a while loop that depends on whether there has been a click/a live key pressed and the max_wait.

Upshot, it should check at the beginning of those functions (but actually doesn't in my test script).

Update: calibration checking doesn't work because wait_secs isn't given an instance of ec as an argument in _init_wait_press or _init_wait_click but this can be changed?

maddycapp27 · 2018-08-02T16:59:15Z

Another question is whether the recalibration should show the calibration prompt or not. Since the default behavior of el.calibrate() is to show the prompt, that is what I have for the moment.

@larsoner @drammock Any concerns so far?

maddycapp27 · 2018-08-03T18:23:14Z

Tested with real eyelink and seems to behave

larsoner · 2018-08-03T18:24:13Z

expyfun/_experiment_controller.py

+
+        Notes
+        -----
+        See `flip_and_play` for order of operations. Can be called multiple


This does not look correct / relevant

larsoner · 2018-08-03T18:25:58Z

expyfun/_eyelink_controller.py

    @verbose_dec
-    def __init__(self, ec, link='default', fs=1000, verbose=None):
+    def __init__(self, ec, link='default', fs=1000, verbose=None,
+                 calibration_key=['c']):


Do not use mutable default arguments. Tuple is immutable so ('c',) is better here

larsoner · 2018-08-03T18:27:21Z

expyfun/tests/test_eyelink_controller.py

        el._fake_calibration = True
        el.calibrate(beep=False, prompt=False)
+        fake_button_press(ec, 'c')
+        el.check_recalibrate(prompt=False)


Shouldn't you assert that this is True ?

The test gets hung up on the prompt screen. Could put in a fake_button_press if you want to test the prompt?

I don't understand. It currently gets stuck, or will get stuck with my suggested addition of an assert in this line preceding the el...?

Read too quickly, you are correct

drammock

I tried to understand what this does just by reading the docstrings and the code, so I have several docstring suggestions. I haven't tested the code itself.

drammock · 2018-08-03T21:28:59Z

expyfun/_experiment_controller.py

+        Parameters
+        ----------
+        function : function | None
+            The function to call. If ``None``, all the "on every flip"


shouldn't this say all the "on every wait"?

drammock · 2018-08-03T21:32:05Z

expyfun/_eyelink_controller.py

    verbose : bool, str, int, or None
        If not None, override default verbose level (see expyfun.verbose).
+    calbration_keys : list
+        Keys that will trigger recalibration when check_recalibration.


can you expand this docstring line a bit? The reference to check_calibration needs context (i.e., it's not a parameter of this constructor, so explain what it is / where it lives)... is it referring to the new check_recalibrate method? (if so, the method name is wrong here)

also, you say it should be list but you initiate it with a tuple. Will it work with any iterable? if so, say iterable instead of list

drammock · 2018-08-03T21:38:43Z

expyfun/_eyelink_controller.py

+    def check_recalibrate(self, keys=None, prompt=True):
+        """Compare key buffer to recalibration keys and calibrate if matched.
+
+        This function always uses the keyboard, so is part of abstraction.


Not sure I understand ths note.

drammock · 2018-08-03T21:42:04Z

expyfun/_eyelink_controller.py

+        Parameters
+        ----------
+        keys : list or string
+            keys to check if prompt recalibration


keys to check against the (set of) key(s) that trigger calibration.

drammock · 2018-08-03T21:43:03Z

expyfun/_eyelink_controller.py

+        keys : list or string
+            keys to check if prompt recalibration
+        prompt : bool
+            Whether to show the calibration prompt or not


Whether to show the calibration prompt screen before starting the calibration procedure

init commit

9a2547b

add check function

76c71a5

add call_on_every_wait

1c17e56

PEP8 and extra lines

387d7f5

maddycapp27 added 2 commits August 3, 2018 13:22

add tests

ada314d

add test to wait_for_fix

2d4d6e9

larsoner reviewed Aug 3, 2018

View reviewed changes

maddycapp27 added 3 commits August 3, 2018 15:01

fix docs

da1a849

very important missing paren

8503b5e

inevitable whitespace

1782482

drammock reviewed Aug 3, 2018

View reviewed changes

maddycapp27 added 3 commits August 5, 2018 12:00

better docs

2863c80

add tests for call_on_every_wait

d331f27

fix coverage maybe

0a068d1

Conversation

maddycapp27 commented Jul 10, 2018

Uh oh!

maddycapp27 commented Jul 10, 2018

Uh oh!

larsoner commented Jul 10, 2018

Uh oh!

maddycapp27 commented Jul 24, 2018

Uh oh!

larsoner commented Jul 24, 2018

Uh oh!

codecov-io commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

maddycapp27 commented Jul 30, 2018

Uh oh!

rkmaddox commented Jul 30, 2018

Uh oh!

rkmaddox commented Jul 30, 2018

Uh oh!

maddycapp27 commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maddycapp27 commented Aug 2, 2018

Uh oh!

maddycapp27 commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jul 30, 2018 •

edited

Loading

maddycapp27 commented Jul 30, 2018 •

edited

Loading