#175194315 Refactor BCInterface, add ExperimentRegistry #111

tab-cmd · 2020-12-07T22:04:15Z

Overview

Refactored BCInterface to select experiments and tasks. Refactored bci_main to accept a task and experiment id. Currently, bci_main and BCInterface do not enforce the field data collection. bci_main does not enforce a valid experiment (see ticket #176044196).

Created ExperimentRegistry.py for creating new experiments. Field creation must be added manually until ticket #175194521.

Currently, registered field names are not viewable in the UI. See ticket #176044412.

Ticket

https://www.pivotaltracker.com/story/show/175194315

Contributions

All changes not described here were formatting updates using autopep8.

Updated

.gitignore: add private .bcipy to prevent custom experiments from being committed.
bci_main.py: to use task and experiment
demo/bci_main_demo: to work with updated bci_main arguments
README: with new bci_main commands and updated UI. Add contributor.
gui/README: with ExperimentRegistry and other updates. Update contributor statement.
tasks/task_registry: updated values to indicate RSVP, add a by_value method. removed by_mode since it is not currently relevant.
tasks/start_task: updates exp_type to task to reflect verbiage elsewhere.
tests/test_save: updated to include task in the init_save_data_structure + added better dt indicator.
tests/test_acquisition: updated to include task in the init_save_data_structure

Added

ExperimentRegistry.py: UI for creating experiments. Field removal, editing, and creation not yet available. On load, it should require name and summary input for creation. It can add any fields in the fields.json after selecting register. It will prevent you from adding the same field twice. Once Create Experiment is selected it will save it in .bcipy/experiment/experiments.json.
helpers/load/: added load experiments and fields methods.
helpers/system_utils/: added default paths and filenames.
helpers/save: added save experiments and fields methods.
tests/test_save: added tests for saving and loading experiments.

Removed

RSVPKeyboard.py: removed in favor of the more generic BCInterface.

Test

Unit tests should pass.
Executing BCInterface, running all experiments in fake data mode using the default experiment. Select the add experiment button from that interface should open the ExperimentRegistry. The registry should function as described in the Contributions section.
bci_main should function as indicated via help text with all flags. defaults are available.

Documentaion

Are documentation updates required? In-line, README, or documentation? Yes. README and in-line documentation updated accordingly.

…arch functionality

… for params_form

lawhead · 2020-12-09T00:27:37Z

demo/bci_main_demo.py

@@ -7,21 +7,20 @@ def main():
    from bcipy.helpers.load import load_json_parameters

    # Load a parameters file
-    parameters = load_json_parameters(
-        'bcipy/parameters/parameters.json', value_cast=True)
+    parameters = 'bcipy/parameters/parameters.json'


We should use the DEFAULT_PARAMETERS_PATH from bcipy.helpers.parameters here.

lawhead · 2020-12-09T00:29:58Z

bcipy/tasks/task_registry.py

@@ -31,13 +31,13 @@ class ExperimentType(Enum):
    'Calibration'
    """


This doctest should be updated as well. The returned value should be 'RSVP Calibration'. Also, should this enum be renamed to TaskType?

I think that makes sense!

lawhead · 2020-12-09T00:32:55Z

bcipy/helpers/save.py

+def save_field_data(data, location, name) -> str:
+    return save_json_data(data, location, name)
+
+


It doesn't seem like these functions add much value. Maybe we just need to make the documentation for save_json_data more generic?

I wanted a good abstraction, so if we changed how we loaded or validated the function wouldn't have to change.

lawhead · 2020-12-09T00:34:23Z

bcipy/helpers/load.py

+    """
+    with open(path, 'r') as json_file:
+        return json.load(json_file)
+


These functions don't guarantee that the data will have the documented fields. The structure is assumed in a number of places but not enforced. It may be worth doing a check on load or converting the JSON to a namedtuple or object.

I have a validate function in the next PR for Experiment Fields! I had to break this up after it started getting bulky.

lawhead · 2020-12-09T00:35:59Z

bci_main.py

 from bcipy.helpers.parameters import DEFAULT_PARAMETERS_PATH
 from bcipy.helpers.save import init_save_data_structure, DEFAULT_EXPERIMENT_ID
 from bcipy.tasks.start_task import start_task
 from bcipy.tasks.task_registry import ExperimentType


-def bci_main(parameter_location: str, user: str, exp_type: int, mode: str, experiment: str = DEFAULT_EXPERIMENT_ID) -> bool:
+def bci_main(parameter_location: str, user: str, task: int, experiment: str = DEFAULT_EXPERIMENT_ID) -> bool:


Looks like the type annotation for the task parameter is inconsistent with the documentation.

lawhead · 2020-12-09T00:37:01Z

bci_main.py



-def execute_task(task_type: dict, parameters: dict, save_folder: str) -> bool:
+def execute_task(task: str, parameters: dict, save_folder: str) -> bool:


The documentation for this method needs to be updated as well.

lawhead · 2020-12-09T00:39:21Z

bci_main.py

@@ -166,11 +158,11 @@ def _clean_up_session(display, daq, server):
    parser.add_argument('-p', '--parameters', default='bcipy/parameters/parameters.json',
                        help='Parameter location. Must be in parameters directory. Pass as parameters/parameters.json')
    parser.add_argument('-u', '--user', default='test_user')
-    parser.add_argument('-t', '--type', default=1,
+    parser.add_argument('-t', '--task', default='RSVP Calibration',


The task_options should be updated to only include the names. We were previously inputting the integer value. Also, we may want to quote these names so it's clear what should be included.

lawhead · 2020-12-09T00:41:09Z

bci_main.py

    args = parser.parse_args()

    # Start BCI Main
-    bci_main(args.parameters, str(args.user), int(args.type), str(args.mode))
+    bci_main(args.parameters, str(args.user), str(args.task), str(args.experiment))


Now that we're passing in the task name rather than the index it seems like it would be easy to pass in a wrong value. We could attempt to get the ExperimentType here so we fail fast if it's missing. It would also simplify the type annotations in the other methods.
bci_main(args.parameters, str(args.user), ExperimentType.by_value(args.task), str(args.experiment))

I like the idea of failing faster. I'll do that. In the next PR, I validate the experiment first thing as well in order to fail before any other processes are started.

lawhead

This looks great!

…uilding other GUIs

Update task options and use DEFAULT_PARAMETERS_PATH as default fail fast by calling experiment type by value after argsparse update documentation update demo and BCInterface refactor experiment type to task type

Params GUI Refactor

Update task options and use DEFAULT_PARAMETERS_PATH as default fail fast by calling experiment type by value after argsparse update documentation update demo and BCInterface refactor experiment type to task type

lawhead added 4 commits November 23, 2020 16:56

Params Form GUI refactor from wxpython to pyqt5

59168e6

Refactored params form to use more object-oriented approach; added se…

a66d9dd

…arch functionality

Code cleanup

9072857

Merged latest code

9bf0e05

tab-cmd changed the title ~~Refactor BCInterface, add ExperimentRegistry~~ #175194315 Refactor BCInterface, add ExperimentRegistry Dec 7, 2020

tab-cmd requested review from lawhead and AlisterD December 7, 2020 22:04

Refactor BCInterface, add ExperimentRegistry

66e7520

tab-cmd force-pushed the experiment-gui branch from 36d6adb to 66e7520 Compare December 7, 2020 22:53

lawhead added 2 commits December 8, 2020 10:05

Merge latest from 1.4.3.

4f03d71

Factored widgets from params_form into gui_main; created a new module…

8f8a729

… for params_form

lawhead reviewed Dec 9, 2020

View reviewed changes

lawhead approved these changes Dec 9, 2020

View reviewed changes

lawhead and others added 6 commits December 9, 2020 16:13

Refactored FormInput elements to be more generic in preparation for b…

7849e8e

…uilding other GUIs

Address PR comments

629358e

Update task options and use DEFAULT_PARAMETERS_PATH as default fail fast by calling experiment type by value after argsparse update documentation update demo and BCInterface refactor experiment type to task type

Merge pull request #109 from CAMBI-tech/params-gui-refactor

22d4231

Params GUI Refactor

Refactor BCInterface, add ExperimentRegistry

b5907fb

Address PR comments

299ab4f

Update task options and use DEFAULT_PARAMETERS_PATH as default fail fast by calling experiment type by value after argsparse update documentation update demo and BCInterface refactor experiment type to task type

Merge remote-tracking branch 'origin/experiment-gui' into experiment-gui

489f883

tab-cmd merged commit 524ee4c into 1.4.3 Dec 10, 2020

tab-cmd deleted the experiment-gui branch January 12, 2021 16:53

tab-cmd mentioned this pull request Jan 20, 2021

1.5.0 #96

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#175194315 Refactor BCInterface, add ExperimentRegistry #111

#175194315 Refactor BCInterface, add ExperimentRegistry #111

tab-cmd commented Dec 7, 2020 •

edited

Loading

lawhead Dec 9, 2020

lawhead Dec 9, 2020

tab-cmd Dec 9, 2020

lawhead Dec 9, 2020

tab-cmd Dec 9, 2020

lawhead Dec 9, 2020

tab-cmd Dec 9, 2020

lawhead Dec 9, 2020

lawhead Dec 9, 2020

lawhead Dec 9, 2020

lawhead Dec 9, 2020

tab-cmd Dec 9, 2020

lawhead left a comment

		@@ -31,13 +31,13 @@ class ExperimentType(Enum):
		'Calibration'
		"""

		def save_field_data(data, location, name) -> str:
		return save_json_data(data, location, name)



		def execute_task(task_type: dict, parameters: dict, save_folder: str) -> bool:
		def execute_task(task: str, parameters: dict, save_folder: str) -> bool:

#175194315 Refactor BCInterface, add ExperimentRegistry #111

#175194315 Refactor BCInterface, add ExperimentRegistry #111

Conversation

tab-cmd commented Dec 7, 2020 • edited Loading

Overview

Ticket

Contributions

Updated

Added

Removed

Test

Documentaion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawhead left a comment

Choose a reason for hiding this comment

tab-cmd commented Dec 7, 2020 •

edited

Loading