First pass at large restructuring

[smathot]

Hi Edwin,

This is a first pass at adding some more structure to the PyGaze codebase. Essentially, all classes have been systematically split up across separate files and organized into separate packages. I think this was necessary to make the code more readable and maintainable, and also to add extra functionality later on (notably integration with OpenSesame).

There are some changes in the API, but everything should be backwards compatible as well. For example, you can now say ...

from pygaze import Display, Screen

... but the old style still works as well ...

from pygaze.libscreen import Display, Screen

That being said, I'm pretty sure that I broke a lot of stuff with this reorganization, particularly the eyetracker stuff, which I can't test from home. These things should be easy to fix though. So don't consider this a fully functional and polished pull request: I just wanted to let you know already to avoid working on completely different branches that will be difficult to merge back together later on.

Cheers, Sebastiaan

[smathot]

PS. Maybe it's best to merge this into a separate branch for now. I'm working from the modulatization branch.

[smathot]

As you know, I think it would be a great idea to move the OpenSesame eye-tracking functionality to PyGaze, by creating a set of pygaze_[x] plug-ins for OpenSesame. I think this should be fairly easy to do, but before I start adding the necessary bits for integration with OpenSesame I have some suggestions and questions.

Some important stuff

Documentation

I see that the API is already nicely documented online, which is great. How does this work? Do you have a system in place for auto-generating the docs, or is there still a bit of manual labor involved?

The tricky thing with having different classes that implement the same functionality (e.g., PsychoPyDisplay and PyGameDisplay) is to decide where the docstrings are going to be. Right now it looks like docstrings are duplicated, but that's probably not a good idea with an eye to maintainability: You don't want to make every change twice.

What we could do is add a base class to each package, for example pygaze.display.basedisplay.BaseDisplay. This would then be a class with only empty function definitions (i.e. containing only pass) and docstrings. These docstrings can then be used for generating the documentation. The docstrings of PsychoPyDisplay etc. can then be reduced to something like 'see BaseDisplay'.

Although it doesn't really make a difference it would also make sense to have the other classes inherit the base class, like so:

class PsychoPyDisplay(BaseDisplay):
    ...

And analogous for the other classes, of course.

It would still be a bit tricky to deal with the fact that the constructors do not all accept the same keywords. For example, how do we document that libeyelink.__init__() accepts a data_file keyword, but libsmi.__init__() does not? The easiest option would simply be to add all of this information into the docstring of a basetracker.BaseTracker.__init__(), but there may be more elegant ways to do this.

What do you think?

No unused keywords

You will see that EyeTracker.__init__() does not specify all possible keywords, but simply captures all keywords in a dictionary **args, which is passed on to the relevant constructor. The same trick is used elsewhere. The obvious benefit here is that the EyeTracker class can be agnostic about the keywords that the eye-tracker-specific classes require.

A sensible and consistent API

As you can see, I messed around with the API. All the restructuring that I did can also be done while keeping the old API, and this up to you. But I think this would a good time to think about what the most sensible API would be from the user's perspective.

Right now there are more multiple classes in the same module, so you can for example do ...

from pygaze.libscreen import Display, Screen
from pygaze.libinput import Keyboard, Mouse
from pygaze.libtime import get_time

... where Display and Screen are classes and get_time is a function. There are two things that are, in my opinion, a bit counterintuitive here. Firstly, the classes are not consistently named after the modules that they are in. Secondly, some modules contain functions (e.g., get_time()), whereas the other modules contain classes (e.g., Screen).

I think it might be better to have a consistent from pygaze.[module] import [class] API, like so:

from pygaze.display import Display
from pygaze.screen import Screen
from pygaze.keyboard import Keyboard
from pygaze.mouse import Mouse
from pygaze.time import Time

With some clever imports in pygaze/__init__.py we can even offer shortcuts:

from pygaze import Display, Screen, Keyboard, Mouse, Time

To only thing that stands in the way at the moment is the fact that libtime is not a class. There is no functional reason to turn this into a class, I know, but it might be consistent to nevertheless do so.

What do you think? Right now would be a good time to choose the perfect API, because when people start using it on a large scale we're pretty much stuck.

Extra functionality, such as libgazecon

libgazecon seems like very high-level functionality. Perhaps it would be cleaner to create a misc or extra package and move libgazecon (and later maybe other packages as well) there?

Some less important stuff

DUMMYMORE vs TRACKERTYPE

defaults.TRACKERTYPE is currently 'eyelink'. Maybe it's better to set it to 'dummy' to avoid a bias towards one brand over another. Also, what's the reasoning behind having a 'dummy' TRACKERTYPE as well as a defaults.DUMMYMODE option?

Really dumb dummy mode

Perhaps there could be pygaze._eyetracker.libreallydumbtracker.py that functions as the old non-interactive dummy mode of the OpenSesame EyeLink plug-ins? This can be convenient sometimes, I kind of liked it for quick debugging.

Abort key

The fact that there is no abort key is pretty annoying while testing. Perhaps escape is too obvious, but I think that some key or sequence of keys should allow you to abort the experiment.

[esdalmaijer]

Hi Sebastiaan,

Nice work! I'm looking at this over the weekend (finally managed to get round to it). As far as I can tell from initial inspection, you didn't break all that much, actually. Full report later on.

Some initial responses to the points you raise:

API

I like the from pygaze import Class setup, as this allows for really easy importing without bothering with specific modules. Also, the current structure does indeed allow for easier maintenance, and the addition of future functionality. What I am less fond of is introducing classes, where good-'ol functions would prove to be perfectly good (e.g. get_time. Let me think about this one, and test some stuff out.

documentation

Ah, yes, this was a rather labourous job, half coded and half manual. As for the online documentation: it's not that big a deal to write a script to extract the docstrings from the source, then create a basic HTML file and insert the documentation in the proper places. Concerning the docstrings within the scripts: a base class inheritance would solve this, but would introduce a problem for user that work via intelligent script editors (e.g. Spyder, but even less sophisticated editors can do it) that show the doc string as you program. If e.g. PyGameDisplay would inherit BaseDisplay, then go on and redefine all functions, the doc would always read "see BadeDisplay", which is rather unhelpful. I suppose we could try to use this to do a docstring inheritance. If that works, I'm happy with the parent class

high level functionality

I suppose we could create a plugin directory, which could contain the gaze contingent functionality, and be the place to add extra modules like libwebcam.

dummy mode

The DUMMYMODE constant is simply an easy way of setting the trackertype to dummymode, originating from before the trackertype keyword was introduced. I just left it in there for backwards compatibility, and to not retype the brand each time ('True' and 'False' are shorter to type that 'eyelink'/'tobii' and 'dummy'; although this argument would be rubbish if you were using 'smi'). I suppose we could take it out, but would leaving it in result in issues?

Agree on the dumb dummymode, that's a good addition!

abort key

Good point. The main reason it's not in there, is that running scripts usually are quite easy to kill if you notice that they don't do as you thought they would (e.g. 'Alt+Tab', then 'Ctrl+C' on Windows), and that users could implement their own 'kill switch' for if stuff goes wrong. Also, I don't like having a single abort key, as participants are sometimes quite keen on hitting it (did you ever test children?). A key combination might be a good idea, though. How do you propose we implement this? Run a parallel Thread that continuously checks for the key sequence?

As I said, more later on, after I've tested this baby. Thanks for your effort and ideas!

[esdalmaijer]

To sum up: pretty much all (with the exception of tracker stuff) of your restructuring is now tested, fixed, and stabilized. I've improved the backwards compatibility as well. I'll upload the testing script, so you can test at your own setup too, after you've changed something. It's quite neat: every non-eyetracker core function is in there.

Anyhow, I'm quite happy with the way this turned out! Some things that still need attention, in order of priority (EDIT: update on 22-Dec-2013; a lot of the todo-stuff has been done and is now listed below as 'fixed', what's left is still listed below as 'todo'):

FIXED

eyetracker testing

I'm testing this on an SMI tomorrow, and I'm hoping to find a stray Tobii somewhere. I won't have access to an EyeLink anytime soon, though. Would you, @smathot ? I do not expect much problems, as the scripts all work in a similar fashion, so if the SMI works, so should the others.

libgazecon and other additional functionality

I still need to add a plugin module, containing the ROI, FRL, and GazeCursor classes. Also, this would be the place for the new webcam functionality and all other future non-standard additions to PyGaze.

stupid dummy

Not a lot of work; will implement tomorrow.

TODO

update documentation

Both online and in the scripts.

abort key

Will look at this tomorrow, if I get to it. Not a big priority at the moment, though. If you have suggestions, please do tell.

[esdalmaijer]

All right, I've fixed and tested the current stuff on both the simulator dummy and an SMI tracker; both seem to work fine. Also implemented is the stupid dummy, which does nothing (it simply passes where no output is required, or returns some default values when output is required). What's left to do now is to update the documentation, both in the scripts and online. And looking at what we can do about the abort key sequence.

[smathot]

Ok, things are starting to look good! I'll start playing around with an OpenSesame plug-in and when I get back to Marseille also check to see whether things work on the EyeLink.

Regarding the documentation. I hadn't really considered that docstrings are also used by editors like Spyder. I think the decorator method is a bit messy, but it wouldn't be very difficult to simply copy the docstrings from one class to another when the method is instantiated (i.e. in __init__()). Below I posted a quick proof of concept.

from pygaze._display.pygamedisplay import PyGameDisplay
from pygaze._display.psychopydisplay import PsychoPyDisplay
from inspect import ismethod

def copy_docstr(src, target):

    """
    Copies docstrings from the methods of a source class to the methods of a
    target class.

    Arguments:
    src     --  The source class.
    target  --  The target class.
    """

    for attr_name in dir(target):
        if not hasattr(src, attr_name) or not ismethod(getattr(src, attr_name)):
            print 'Skipping %s' % attr_name
            continue
        print 'Copying %s' % attr_name
        getattr(target, attr_name).__func__.__doc__ = getattr(src, \
            attr_name).__func__.__doc__

copy_docstr(PyGameDisplay, PsychoPyDisplay)

[esdalmaijer]

Nice, that actually works and looks a lot cleaner than the approach I linked to indeed!