esdalmaijer / PyGaze

an open-source, cross-platform toolbox for minimal-effort programming of eye tracking experiments
www.pygaze.org
GNU General Public License v3.0
671 stars 211 forks source link

Update libeyelink.py, avoids div by zero error in calibration #51

Closed Brixius closed 8 years ago

Brixius commented 8 years ago

Updates libeyelink.py to avoid division by zero crash error possible in RMS/noise calibration. Previously, if participant moved away from eyetracker before RMS recording began (or eye was "lost"), since nothing would be recorded, len(Xvar) and len(Yvar) would be 0 and the program would crash when attempting to divide by 0. This just adds a check. If things went smoothly, it proceeds as before. If nothing was recorded, it displays a message saying so, then returns to the calibration screen for a retry after a space bar press. Apologies if this isn't quite elegant, but it does appear functional!

smathot commented 8 years ago

Thanks! It looks okay (haven't tested though), but in terms of style I have two comma-queen remarks--more for educational purposes than anything else. This construction:

if len(Xvar) != 0 and len(Yvar) != 0:
...
elif len(Xvar) == 0 or len(Yvar) == 0:
...

... is ugly, mostly because the second elif is (in this context) equivalent to an else. More Pythonic and less redundant would be:

if not Xvar or not Yvar:
... # fail
else:
... # success

Or better yet (flipping the blocks around):

if Xvar and Yvar:
... # success
else:
... # fail

Also, by calling calibrate() again when calibration fails, you use recursion, whereas I think that a while loop that redoes part of the calibration would suffice.

My suggestion would be to tidy the code up a little bit, and then we merge. @esdalmaijer?

Brixius commented 8 years ago

Thanks! I'm still pretty new to Python and just recently hit the point where I start refining my old code, so I greatly appreciate the suggestions. I'll change things around and test it out on my end, then re-up (also brand new to GitHub, so not sure if that would entail a new pull request or if I can modify the existing commit here...will sort it once the code is shiny).

esdalmaijer commented 8 years ago

Sorry I'm a bit slow to respond to this. Changes look good, thanks!