Parameter guardrails and cleanup #340
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
update gui code after TaskRegistry refactor
lawhead
approved these changes
Aug 23, 2024
| "helpTip": "If true, fake data server used", | ||
| "recommended_values": "", | ||
| "recommended": "", | ||
| "editable": "true", |
There was a problem hiding this comment.
I wasn't sure where exactly to leave this feedback but the value for editable should just be true or false, rather than the string values ("true", "false"). The value field has to be a string, since it might be cast to any python type, but this one doesn't need casting and JSON can represent booleans.
| @@ -128,7 +128,7 @@ class RSVPCopyPhraseTask(Task): | |||
| "max_inq_per_series", | |||
| "max_minutes", | |||
| "max_selections", | |||
| "min_inq_len", | |||
| "min_inq_per_series", | |||
| "show_feedback", | |||
There was a problem hiding this comment.
This came up in a discussion in February and I created a different ticket for it: https://www.pivotaltracker.com/n/projects/2460065/stories/187147945.
There was a problem hiding this comment.
We did duplicate this! We went with a different solution, but it still addresses the need for a single parameter.
| helpTip: str | ||
| recommended_values: list | ||
| recommended: list | ||
| editable: bool |
There was a problem hiding this comment.
I think this improves readability but it's kind of a major change. These parameters files tend to get used for various things long after data is collected (training models, directing the simulator in how to parse the raw data, etc.). We may want to provide a script to upgrade existing parameters files or add a compatibility layer which can read the old format.
There was a problem hiding this comment.
I am leaning toward a compatibility layer since this will only be used online and we haven't released a new version in awhile. It can be safely ignored like helpTip in many cases.
tab-cmd
added a commit
that referenced
this pull request
Oct 23, 2024
* gets `main.py` functionality working in the orchestrator * Fixes task initialization process Save folder and parameters are now saved and loaded per task as they are executed rather than once per session. * Moves parameter loading to task execution * Adds functions for parsing visit actions * Adds some test to action parsing * Adds tests for action serialization * Adds parameter validation function with tests * Updates changelog.md * lints actions.py and test_actions.py * Adds constant for action separator * removes old file that has been moved * renames action parsing module and updates test imports * simplifies `serialize_actions` function * Fixes typing and formatting issues * removes `parse_actions.py` * renames `parse_actions` to `parse_sequence` * replaces magic '->' string with constant * removes unnecessary error check * Adds basic structure for orchestrator actions * refactors config function naming and adds support for actions in parsing functions * renames `test_actions` to `test_config` * Adds additional tests for action parsing * Adds action serialization tests * Add more actions * Refactors type unions to be `Optional`s * Adds parameter loading to the orchestrator * Adds initialize and cleanup methods to `Task` * makes `name` a property in `OfflineAnalysisAction` * Implements `setup` method in base `Task` class * Adds missing parent `execute` call * changes `initialize` call to `setup` in orchestrator * Refactors `name` to be a property in base `Task` class * Refactors `config.py` to only work with tasks * adds demo file for orchestrator execution * Fixes data_save initialization in orchestrator * Creates default_user_id constant * Fixes incorrect user initialization * Refactors orchestrator save structure * Implements `save` method * Adds `task_registry_dict` to simplify fetching classes for initialization This is currently redundant with the `TaskType` approach and ideally would be refactored to have only one source of string reference * Adds comment about `task_registry_dict` * Moves `task_registry_dict` to `orchestrator.config` * refactors `name` property of tasks * Adds task registry class * Refactor `super` calls in orchestrator actions * adds type hint for task parameter * calls `construct_command` in constructor * adds function to run experiment data collection without running from the command line * adds `ExperimentFieldCollectionAction` * refactors orchestrator demo file to use `ExperimentFieldCollectionAction` * lints `task_registry.py` * imports `Task` for correct typing in config * Updates `task_registry.py` to import all tasks * moves `TASK_SEPERATOR` to `config.py` * Adds docstring to orchestrator * Renames `TestConfig` * removes property decorator in task class * removes implementation of abstract class * updates changelog * removes super calls to abstract method `execute` * Filters out abstract subclasses from registry generation * removes registry dict empty initialization * Fixes typing issues * Removes task import from the registry * Refactors config to support TaskRegistry * Refactors task names * Refactors `TaskRegistry` to recursively collect subclasses This makes it compatible with the new `BaseCalibrationTask` as it will be able to collect grandchild classes of `Task` (which is all calibration tasks now) * Makes `BaseCalibrationTask` abstract * removes name and label from `timing_verification.py` * imports tasks in their module's `__init__.py` This makes all tasks available to the task registry when it is initialized. * Enforces `name` attribute in base task class * adds calibration task check to the registry. TaskType now no longer exists * fixes incorrect method name in TaskRegistry * updates bci_main_tests to call main methods properly * updates method calls in protocol parsing tests * refactors `start_app` to use `start_experiment_field_collection_gui` * changes orchestrator directory name to work on windows * Adds basic selection box for orchestrator protocol * Adds the ability to add and remove tasks to the protocol * renames `sequence` to `protocol` in orchestrator config * Allows protocol to be saved to `experiments.json` * adds move up and down controls to tasks in protocol * Refactors scrollable frames to have a title label. Some buttons are still misaligned in the ExperimentRegistry * Moves create experiment button to the bottom of the gui * changes task and action names to be more consistent * adds basic `TaskData` return object * fixes failing test in `test_copy_phrase` * fixes missing return statement for `construct_command` * adds `task_dict` to `TaskData` * updates changelog * adds seperate BCIUI file with stylsheet This will hopefully replace BCIGui later, but I am keeping it seperate for now as to not break existing uis on this branch * update demo experiment layout * adjust stylesheet to remove label backgrounds * improves gui code to allow for editing fields * add basic removeable element * add dynamic list class for easy ui reactivity * Add small button component * removes explicit color from small button * adds basic toggle class * changes toggle componenet to be simple connecting function * makes on off callbacks optional in `make_toggle` * add toggle to optional button * swaps confusing parameter names * loads experiment fields into combobox * adds field buttons * refactors gui to be inherited class This improves state handling, as variables and widgets can be stored on the class itself * add simple `show_alert` method * add show_alert method * adds validation to added fields * adds experiment creation to application * rename `gui_rewrite` to `bciui` * adds app start method * adds vertical center option to bciui * add the beginning of intertask action ui * add progress text * allows for additional args in app running * add functional progress bar * Add next task display and functioning buttons * adds seperate BCIUI file with stylesheet This will hopefully replace BCIGui later, but I am keeping it seperate for now as to not break existing uis on this branch * update demo experiment layout * adjust stylesheet to remove label backgrounds * improves gui code to allow for editing fields * add basic removeable element * add dynamic list class for easy ui reactivity * Add small button component * removes explicit color from small button * adds basic toggle class * changes toggle componenet to be simple connecting function * makes on off callbacks optional in `make_toggle` * add toggle to optional button * swaps confusing parameter names * loads experiment fields into combobox * adds field buttons * refactors gui to be inherited class This improves state handling, as variables and widgets can be stored on the class itself * add simple `show_alert` method * add show_alert method * adds validation to added fields * remove debug prints * adds experiment creation to application * remove unused code * rename `gui_rewrite` to `bciui` * adds app start method * adds vertical center option to bciui * add beginning of intertask action ui * add progress text * allows for additional args in app running * add functional progress bar * Add next task display and functioning buttons * add protocol layout and fix bug in DynamicList * Parameter guardrails and cleanup (#340) * supports adding tasks to the interface * add reordering to tasks in the protocol * allows adding new fields through the ui * adds saving protocol in experiment from gui * Task initialization and cleanup (#343) * Adds documentation to ui methods * rename `intertask_action` to `intertask_gui` * add action that displays intertask GUI * Add proper types and docs to bciui * Deprecates old ExperimentRegistry in favor of new ui code * Fixes incorrect background color when using dark mode * improve stylsheet loading to use existing bcipy constants * update legacy BCInterface to base session-orchestrator * Session Orchestrator Updates [Post-Testing] (#344) * Add logger to each task run * Bcipy client refactor (#346) * Task specific params and Matrix Layout (#349) * Orchestrator - Load a JSON of copy phrases to use during execution (#352) * fix logger assignment * BciPy report action, offline analysis updates, Intertask GUI. (#354) * updates to include IP changes * Update to get static offset from the device --------- Co-authored-by: Carson Reader <[email protected]> Co-authored-by: lawhead <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Add
editableboolean field to parameters.json entries, update GUI, and make small adjustments to the parameter form for readability. Refactored file names in Task to have more consistency across modules. This field will eventually be used by the Tasks and Session Orchestrator to prevent any parameter updates if editable is false.Ticket
https://www.pivotaltracker.com/story/show/187727881
https://www.pivotaltracker.com/story/show/188144688
Contributions
Test
Documentation
Changelog