-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Experiment class refactor #183
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
test = Experiment("Visual N170") | ||
test.instruction_text = instruction_text | ||
test.load_stimulus = load_stimulus | ||
test.present_stimulus = present_stimulus | ||
test.setup() | ||
test.present() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do it like this? I assumed you'd create a subclass of Experiment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
"""Initialize flickering stimulus. | ||
Get parameters for a flickering stimulus, based on the screen refresh | ||
rate and the desired stimulation cycle. | ||
Args: | ||
frame_rate (float): screen frame rate, in Hz | ||
cycle (tuple or int): if tuple (on, off), represents the number of | ||
'on' periods and 'off' periods in one flickering cycle. This | ||
supposes a "single graphic" stimulus, where the displayed object | ||
appears and disappears in the background. | ||
If int, represents the number of total periods in one cycle. | ||
This supposes a "pattern reversal" stimulus, where the | ||
displayed object appears and is replaced by its opposite. | ||
soa (float): stimulus duration, in s | ||
Returns: | ||
(dict): dictionary with keys | ||
'cycle' -> tuple of (on, off) periods in a cycle | ||
'freq' -> stimulus frequency | ||
'n_cycles' -> number of cycles in one stimulus trial | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore the visual_ssvep for now, still doing it. But I'm gonna keep that in the load_stimulus or present_stimulus function
"""Get possible SSVEP stimulation frequencies. | ||
Utility function that returns the possible SSVEP stimulation | ||
frequencies and on/off pattern based on screen refresh rate. | ||
Args: | ||
frame_rate (float): screen frame rate, in Hz | ||
Keyword Args: | ||
stim_type (str): type of stimulation | ||
'single'-> single graphic stimulus (the displayed object | ||
appears and disappears in the background.) | ||
'reversal' -> pattern reversal stimulus (the displayed object | ||
appears and is replaced by its opposite.) | ||
Returns: | ||
(dict): keys are stimulation frequencies (in Hz), and values are | ||
lists of tuples, where each tuple is the number of (on, off) | ||
periods of one stimulation cycle | ||
For more info on stimulation patterns, see Section 2 of: | ||
Danhua Zhu, Jordi Bieger, Gary Garcia Molina, and Ronald M. Aarts, | ||
"A Survey of Stimulation Methods Used in SSVEP-Based BCIs," | ||
Computational Intelligence and Neuroscience, vol. 2010, 12 pages, | ||
2010. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore the visual_ssvep for now, still doing it. But I'm gonna keep that in the load_stimulus or present_stimulus function
self.iti = 0.4 | ||
self.soa = 0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if eeg.backend == "muselsl": | ||
marker = [markernames[label]] | ||
else: | ||
marker = markernames[label] | ||
eeg.push_sample(marker=marker, timestamp=timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohnGriffiths This muselsl-edgecase should prob be handled directly in push_sample
, and not in calling code. (not related to this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I think you should definitely focus on one or two experiments and get them working before you start modifying the rest. |
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. |
…iments to subclasses (#184) * First commit * Second commit * Modifications * Lol * Lol * Incorporated N170 and p300, looking good for a PR * ssvep update * Implementing subclasses instead of loose functions * fix: fixed import (brainflow updated API) * Playing around still * Fixing import errors * Adding abstractmethod decorators * Still working on the import error * Guess what's finally working * Comments and naming ticks * More comments * Live coding demonstration * ssvep adapted * Adapting Auditory Oddball * changing save_fn to self.save_fun * This maybe the last big change * utils file changed, changes work through cli Co-authored-by: Erik Bjäreholt <erik@bjareho.lt>
…ific experiments to subclasses (NeuroTechX#184) * First commit * Second commit * Modifications * Lol * Lol * Incorporated N170 and p300, looking good for a PR * ssvep update * Implementing subclasses instead of loose functions * fix: fixed import (brainflow updated API) * Playing around still * Fixing import errors * Adding abstractmethod decorators * Still working on the import error * Guess what's finally working * Comments and naming ticks * More comments * Live coding demonstration * ssvep adapted * Adapting Auditory Oddball * changing save_fn to self.save_fun * This maybe the last big change * utils file changed, changes work through cli Co-authored-by: Erik Bjäreholt <erik@bjareho.lt>
* example test commit (#182) * example test commit * example edit * ci: run test workflow on develop branch * ci: add develop branch to job triggers * ci: fix syntax issue in workflow * fix: fixed import (brainflow updated API) * build(deps): locked pylsl==1.10.5 (#187) * Experiment Class Refactor (update to #183), converting specific experiments to subclasses (#184) * First commit * Second commit * Modifications * Lol * Lol * Incorporated N170 and p300, looking good for a PR * ssvep update * Implementing subclasses instead of loose functions * fix: fixed import (brainflow updated API) * Playing around still * Fixing import errors * Adding abstractmethod decorators * Still working on the import error * Guess what's finally working * Comments and naming ticks * More comments * Live coding demonstration * ssvep adapted * Adapting Auditory Oddball * changing save_fn to self.save_fun * This maybe the last big change * utils file changed, changes work through cli Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Submodule added for gsoc * Adding pipelines for cli analysis (#202) * started pipelines function * almost working simple function equivalents of nb scripts * fix: fixed import (brainflow updated API) * sqc fixes for unicorn (#176) * Ignore pushes * Trying to create a cli * Stepping through the problem * First commit * Fixing pause in signal quality check * Fixing Signal quality check problem * fix the technical debt * Save path done for automated saving pdf * I feel amazing * Almost through * Update eegnb/cli/__main__.py Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Trying to create cli but it's being really painful * Extra word cli error * Changed example handling * Pain * Adding whole datapath * Finally fixed cli * hmm * Looking good * added hyperlink * Having some issues with detecting css and image deltetion * Just the css now * Fixed the css linking problem though it's a weird soln * Automated running, still fnames problem * Hahahah embedded images in html * Improving code * Okay now * Look at that * Almost there just the two figures now * Now * Added attrdict to do with cli error Co-authored-by: John Griffiths <j.davidgriffiths@gmail.com> Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * added more options for site args; improved function names; removed some redundant lines (#209) * fix subject num parsing bug * analysis report function improvements for openbci cyton and gtec unicorn devices * run exp fix * Update requirements.txt * fixes to get docs building by github action (#210) * fixes to get docs building by github action * reverted some changes * Update 01r__ssvep_viz.py Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * Update README.rst small commit to test doc build workflow on this branch * removing gsoc submodule Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: Parv Agarwal <65726543+Parvfect@users.noreply.github.com> Co-authored-by: Parvfect <parvagrw02@gmail.com> Co-authored-by: Ben Pettit <pelleter@gmail.com>
* major update: merging develop to master (#217) * example test commit (#182) * example test commit * example edit * ci: run test workflow on develop branch * ci: add develop branch to job triggers * ci: fix syntax issue in workflow * fix: fixed import (brainflow updated API) * build(deps): locked pylsl==1.10.5 (#187) * Experiment Class Refactor (update to #183), converting specific experiments to subclasses (#184) * First commit * Second commit * Modifications * Lol * Lol * Incorporated N170 and p300, looking good for a PR * ssvep update * Implementing subclasses instead of loose functions * fix: fixed import (brainflow updated API) * Playing around still * Fixing import errors * Adding abstractmethod decorators * Still working on the import error * Guess what's finally working * Comments and naming ticks * More comments * Live coding demonstration * ssvep adapted * Adapting Auditory Oddball * changing save_fn to self.save_fun * This maybe the last big change * utils file changed, changes work through cli Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Submodule added for gsoc * Adding pipelines for cli analysis (#202) * started pipelines function * almost working simple function equivalents of nb scripts * fix: fixed import (brainflow updated API) * sqc fixes for unicorn (#176) * Ignore pushes * Trying to create a cli * Stepping through the problem * First commit * Fixing pause in signal quality check * Fixing Signal quality check problem * fix the technical debt * Save path done for automated saving pdf * I feel amazing * Almost through * Update eegnb/cli/__main__.py Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Trying to create cli but it's being really painful * Extra word cli error * Changed example handling * Pain * Adding whole datapath * Finally fixed cli * hmm * Looking good * added hyperlink * Having some issues with detecting css and image deltetion * Just the css now * Fixed the css linking problem though it's a weird soln * Automated running, still fnames problem * Hahahah embedded images in html * Improving code * Okay now * Look at that * Almost there just the two figures now * Now * Added attrdict to do with cli error Co-authored-by: John Griffiths <j.davidgriffiths@gmail.com> Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * added more options for site args; improved function names; removed some redundant lines (#209) * fix subject num parsing bug * analysis report function improvements for openbci cyton and gtec unicorn devices * run exp fix * Update requirements.txt * fixes to get docs building by github action (#210) * fixes to get docs building by github action * reverted some changes * Update 01r__ssvep_viz.py Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * Update README.rst small commit to test doc build workflow on this branch * removing gsoc submodule Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: Parv Agarwal <65726543+Parvfect@users.noreply.github.com> Co-authored-by: Parvfect <parvagrw02@gmail.com> Co-authored-by: Ben Pettit <pelleter@gmail.com> * update dependencies - seaborn * docs/perf: reduced the imports: `cueing` example * bug: update deprecated `plot_psd()` method - feature of new `mne` version. - instead of doing plot_psd() from the `mne.io.Raw` object, must do this: - `raw.compute_psd().plot()` - i.e., has to pass through a `spectrum` object * updated deprec. `mne` function * perf: removed importage of unused packages from example - One of them, i.e., `collections.Iterable` is even deprecated. - Must use `collections.abc.Iterable` instead now. - Resulting in faster build/user run * bugfix: `plot_conditions` - due to `sns` deprecation * bugfix: resolved `psd_welch()` deprecation (`mne`) --------- Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: Parv Agarwal <65726543+Parvfect@users.noreply.github.com> Co-authored-by: Parvfect <parvagrw02@gmail.com> Co-authored-by: Ben Pettit <pelleter@gmail.com> Co-authored-by: Taha Morshedzadeh <taha.morshedzadeh@neuromatch.io>
…Refactor (#218) * major update: merging develop to master (#217) * example test commit (#182) * example test commit * example edit * ci: run test workflow on develop branch * ci: add develop branch to job triggers * ci: fix syntax issue in workflow * fix: fixed import (brainflow updated API) * build(deps): locked pylsl==1.10.5 (#187) * Experiment Class Refactor (update to #183), converting specific experiments to subclasses (#184) * First commit * Second commit * Modifications * Lol * Lol * Incorporated N170 and p300, looking good for a PR * ssvep update * Implementing subclasses instead of loose functions * fix: fixed import (brainflow updated API) * Playing around still * Fixing import errors * Adding abstractmethod decorators * Still working on the import error * Guess what's finally working * Comments and naming ticks * More comments * Live coding demonstration * ssvep adapted * Adapting Auditory Oddball * changing save_fn to self.save_fun * This maybe the last big change * utils file changed, changes work through cli Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Submodule added for gsoc * Adding pipelines for cli analysis (#202) * started pipelines function * almost working simple function equivalents of nb scripts * fix: fixed import (brainflow updated API) * sqc fixes for unicorn (#176) * Ignore pushes * Trying to create a cli * Stepping through the problem * First commit * Fixing pause in signal quality check * Fixing Signal quality check problem * fix the technical debt * Save path done for automated saving pdf * I feel amazing * Almost through * Update eegnb/cli/__main__.py Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> * Trying to create cli but it's being really painful * Extra word cli error * Changed example handling * Pain * Adding whole datapath * Finally fixed cli * hmm * Looking good * added hyperlink * Having some issues with detecting css and image deltetion * Just the css now * Fixed the css linking problem though it's a weird soln * Automated running, still fnames problem * Hahahah embedded images in html * Improving code * Okay now * Look at that * Almost there just the two figures now * Now * Added attrdict to do with cli error Co-authored-by: John Griffiths <j.davidgriffiths@gmail.com> Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * added more options for site args; improved function names; removed some redundant lines (#209) * fix subject num parsing bug * analysis report function improvements for openbci cyton and gtec unicorn devices * run exp fix * Update requirements.txt * fixes to get docs building by github action (#210) * fixes to get docs building by github action * reverted some changes * Update 01r__ssvep_viz.py Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> * Update README.rst small commit to test doc build workflow on this branch * removing gsoc submodule Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: Parv Agarwal <65726543+Parvfect@users.noreply.github.com> Co-authored-by: Parvfect <parvagrw02@gmail.com> Co-authored-by: Ben Pettit <pelleter@gmail.com> * Updated doc examples * Update 00x__n170_run_experiment.py fix: typo in func param --------- Co-authored-by: John Griffiths <JohnGriffiths@users.noreply.github.com> Co-authored-by: Erik Bjäreholt <erik@bjareho.lt> Co-authored-by: Ben Pettit <pelleter@gmail.com> Co-authored-by: Ore O <oreogundipe@gmail.com>
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.