Immutable params #101

lawhead · 2020-10-27T16:36:32Z

Overview

In the current workflow, any edits to the configuration for a session overwrite the default parameters. This makes it problematic to upgrade BciPy since this file often gets updated with new configuration options. This same file is used for both defining which parameters are available and their default values, as well as the configuration used for the current session.

This PR separates out these two concepts so that the default parameters.json file never gets overridden when configuration a session. The workflow has been modified so that users in the RSVPKeyboard GUI interface can select an existing parameters file. If they attempt to edit the configuration without first selecting experiment parameters, a new parameters file is created by copying the default params.

Ticket

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

Contributions

Modified RSVPKeyboard GUI to change the workflow for selecting and editing a parameters file. The configured file is passed to both the params_form for editing and bci_main for starting a session.
Added a command line argument to bci_main to accept a parameters file location.
Added functionality for copying a parameters file.
Created a new Parameters class which extends dict. Usage throughout the codebase for accessing configuration is unchanged, but it retains metadata and can be saved as a JSON file in our expected serialization format. It also performs validation of configuration parameters.

Test

Run the unit tests to ensure parameter functionality is unchanged.
In the RSVPKeyboard GUI:
- Attempt to configure parameters when none has been selected. A new parameters file with the current timestamp should be created. After starting a calibration session, the parameters file in the data folder should match the new params file. The parameter_location property should point to the new file.
- Load an existing parameters file.

…meters.json file which will be edited

…rameters.json, rather than override the default values for an experiment

… within the bcipy code base

tab-cmd

A few comments to address. Let's get time with one of the clinical team members (Betts, Deirdre, or Dan) to review the GUI changes.

tab-cmd · 2020-10-27T22:22:08Z

bcipy/helpers/load.py


 from tkinter.filedialog import askopenfilename, askdirectory

 log = logging.getLogger(__name__)

+DEFAULT_PARAMETERS_PATH = 'bcipy/parameters/parameters.json'


It looks like we should pull this from from bcipy.helpers.parameters?

Yes, good point. I'll clean that up.

tab-cmd · 2020-10-27T22:23:00Z

bcipy/helpers/load.py

-def cast_value(value):
-    """Cast Value.
+def copy_parameters(path: str = DEFAULT_PARAMETERS_PATH,
+                    destination: str = 'bcipy/parameters/') -> str:


Let's make this a constant or derived from DEFAULT_PARAMETERS_PATH

tab-cmd · 2020-10-27T22:23:57Z

bcipy/helpers/tests/test_save.py

@@ -12,7 +13,7 @@ class TestSave(unittest.TestCase):
    def setUp(self):
        # set up the needed paths and initial data save structure

-        self.data_save_path = 'data/'
+        self.data_save_path = tempfile.mkdtemp()


tab-cmd · 2020-10-27T22:25:03Z

bcipy/helpers/parameters.py

+DEFAULT_PARAMETERS_PATH = 'bcipy/parameters/parameters.json'
+
+
+class Parameters(dict):


This makes me really happy. I like the parameters being a class that can handle itself.

tab-cmd · 2020-10-27T22:26:51Z

bcipy/helpers/load.py

-    except Exception:
-        raise ValueError(f'Could not cast {actual_value} to {actual_type}')
+    now = datetime.now()
+    month = str(now.month).rjust(2, "0")


I think it's cleaner to use strftime for filenames with datetimes. I use it in helpers/save

Thanks, I'll look at that.

tab-cmd · 2020-10-27T22:27:40Z

bcipy/gui/params_form.py

    frame.Show()
    app.MainLoop()


 if __name__ == '__main__':
-    main()
+    import argparse
+    from bcipy.helpers.load import DEFAULT_PARAMETERS_PATH


I'd be okay loading from bcipy.helpers.parameters

tab-cmd · 2020-10-27T22:28:17Z

bcipy/gui/params_form.py

            size=14)
        loading_box.Add(self.loaded_from)
-        loading_box.AddSpacer(10)
+        # loading_box.AddSpacer(5)


feel free to remove if commented out code isn't needed

Thanks, good catch.

tab-cmd · 2020-10-27T22:28:55Z

bcipy/gui/mode/RSVPKeyboard.py

@@ -45,8 +73,9 @@ def launch_bci_main(self, event: wx.Event) -> None:
            username = self.comboboxes[0].GetValue().replace(" ", "_")
            experiment_type = event.GetEventObject().GetId()
            mode = 'RSVP'
-            cmd = 'python bci_main.py -m {} -t {} -u {}'.format(
-                mode, experiment_type, username)
+            # TODO: add -p flag and use configured parameter_location


looks like you're doing that?

Oh, yep. I'll remove this.

tab-cmd · 2020-10-27T22:29:38Z

bcipy/gui/mode/RSVPKeyboard.py

 import itertools
 import os
 import subprocess

 import wx

 from bcipy.gui.gui_main import BCIGui
-from bcipy.helpers.load import load_json_parameters
+from bcipy.helpers.load import load_json_parameters, copy_parameters
+from bcipy.helpers.load import DEFAULT_PARAMETERS_PATH


I'd take this from helpers/parameters or keep it on the same line

lawhead added 13 commits October 16, 2020 13:54

Modified params_form GUI to accept a command line option for the para…

66f9657

…meters.json file which will be edited

Add parameter_location to RSVPKeyboard

2206698

Merged latest from 1.4.3

2c8a8be

Merged latest from 1.4.3

f58ec3d

Immutable parameters; refactored GUI workflow to use a copy of the pa…

d70352f

…rameters.json, rather than override the default values for an experiment

Fixed init_save_data_structure

a4e5dd1

Parameters class for encapsulating functionality for parameter handling

0e7e81a

Test cleanup; added more unit tests to cover usage of parameters from…

8db13e3

… within the bcipy code base

Prevent test from deleting the data directory

a492031

Refactored to use new Parameters object

95e9dc1

Removed unneccessary exception handling

87643d5

Added save option to parameters.py; increased test coverage

66e3265

Write parameter_location before copying over to the data folder

5f7ed9e

lawhead requested a review from tab-cmd October 27, 2020 16:36

tab-cmd approved these changes Oct 27, 2020

View reviewed changes

Cleanup

d480fb2

lawhead merged commit dfa2e2e into 1.4.3 Oct 28, 2020

tab-cmd mentioned this pull request Jan 20, 2021

1.5.0 #96

Merged

4 tasks

lawhead deleted the immutable-params branch January 29, 2021 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable params #101

Immutable params #101

lawhead commented Oct 27, 2020

tab-cmd left a comment

tab-cmd Oct 27, 2020

lawhead Oct 28, 2020

tab-cmd Oct 27, 2020

tab-cmd Oct 27, 2020

tab-cmd Oct 27, 2020

tab-cmd Oct 27, 2020

lawhead Oct 28, 2020

tab-cmd Oct 27, 2020

tab-cmd Oct 27, 2020

lawhead Oct 28, 2020

tab-cmd Oct 27, 2020

lawhead Oct 28, 2020

tab-cmd Oct 27, 2020

		DEFAULT_PARAMETERS_PATH = 'bcipy/parameters/parameters.json'


		class Parameters(dict):

Immutable params #101

Immutable params #101

Conversation

lawhead commented Oct 27, 2020

Overview

Ticket

Contributions

Test

tab-cmd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment