Experiment class refactor

[Parvfect]

Defined General Experiment Class type that does all of the operations besides loading and presenting the stimulus which is overriden by the derived classes. As of now it incorporates the Visual N170 and Visual P300 experiments - and working towards incorporating Visual SSVEP.

Some things worth thinking about, for the present_stimulus function, a lot of parameters are being passed which is annoying me, so I want to know if it would be smarter to pass the Experiment as an object instead (although I know in general that's bad practice). It also gives us a lot of flexibility for the future complex experiements.

Additionally, since all the main functions are delegated to lower classes, this mechanism should allow a lot of complexity and specificity and generally think this should work (and it looks cool too!).

Has not been tested besides compiling, so that needs to be done once whilst I work towards incorporating the rest of the experiments.

[ErikBjare]

Some comments

[ErikBjare]

Why do it like this? I assumed you'd create a subclass of Experiment?

[Parvfect]

I think this works better because I don't need to care about redefining a bunch of stuff and thinking about defining the functions in the base class etc. This way I can use the same class everywhere and just initialize it differently for the different experiments. And since I am seperating the present_stimulus and load_stimulus functions, it does not make much of a difference

[JohnGriffiths]

I agree with Erik I think the natural way of doing this is with an experiment base class that is subclassed for every experiment. The functions that you are passing here would then be (potentially decorated) methods in the subclasses.

You're right that you're saving some overhead by using the same class everywhere and not needing placeholder function definitions in the base class like

def present_stimulus(self):
  pass

...but that's pretty standard idiomatic python and isn't a huge deal.

I'm still going through this thoroughly but I'd like a bit more spelling out as to why this function passing approach is a better than subclassing from a general code base structure perspective.

[Parvfect]

Yeah I've had a look around and I think it makes more sense to have subclasses cause it can also provide readymade access to the variables from outside. Gonna convert it to that type now.

[ErikBjare]

Just a note/tidbit/lesson on your existing approach, that if you do:

class A:
    def b(self):
        print(self)

def c(self):
    print(self)

a = A()
print(a.b)  # <bound method A.b of <__main__.A object at 0x7f645b76c4c0>>
a.b()       # <__main__.A object at 0x7f6459ecd9c0>
a.c = c
print(a.c)  # <function c at 0x7f645c04c790>
a.c()       # TypeError: c() missing 1 required positional argument: 'self'

In other words, a.b will be bound to the instance (have access to self), but a.c wont, and would need to be bound to get access to its own class members: .https://stackoverflow.com/a/1015405/965332

[ErikBjare]

Why remove this?

[Parvfect]

Ignore the visual_ssvep for now, still doing it. But I'm gonna keep that in the load_stimulus or present_stimulus function

[ErikBjare]

Why remove this?

[Parvfect]

Ignore the visual_ssvep for now, still doing it. But I'm gonna keep that in the load_stimulus or present_stimulus function

[ErikBjare]

@JohnGriffiths Always wondered about these confusing variable names. Are iti and soa abbreviations or something?

In any case, they should probably be given more descriptive names (and note that the values are different in different experiments, so they need to be passed too!)

[Parvfect]

yeah so they are being initialized to default values and then if the experiment needs different ones it can change it after the init (otherwise I could force it to initialize too) - see visual ssvep

[JohnGriffiths]

They are standard technical acronyms in psych / cog neuro:

ITI = Inter-trial interval

SOA = Stimulus onset asynchrony

( A related term that we don't have here is ISI (inter-stimulus interval) )

These absolutely should be more clearly defined and consistently used.

[ErikBjare]

@JohnGriffiths This muselsl-edgecase should prob be handled directly in push_sample, and not in calling code. (not related to this PR)

[JohnGriffiths]

agree.

[JohnGriffiths]

Also the whole push sample part should be inside the experiment base class; in the majority of cases just comes immediately after stim presentation and doesn't need to be repeated

[ErikBjare]

Has not been tested besides compiling, so that needs to be done once whilst I work towards incorporating the rest of the experiments.

I think you should definitely focus on one or two experiments and get them working before you start modifying the rest.

[ErikBjare]

I also just fixed so that CI runs on PRs targeting develop, so if you rebase/merge in those changes you should get CI to run.

[JohnGriffiths]

Closing as this feature was successfully implemented with #183 and is now working well in the core code base