Clean-up Experiment dependencies inside Stimulus #72

fedem-p · 2022-03-01T15:32:04Z

With this PR the protocol runner passes the calibrator object from the method initialise_external().
To avoid problems with existing custom stimuli which may have rewritten the method I added two warnings.

More specifically I expect two cases to throw errors:

if the protocol runner calls a custom stimulus where the initialise_external() function doesn't accept the calibrator input.
if the function initialise_external() is used without passing the calibrator input.

To fix 1:

in the protocol runner there's a try{} except{} block that tries to feed the calibrator object to the stimulus initialise_external() function. If this doesn't work then the protocol runner raises a warning and then calls the function without the calibrator input.

To fix 2:

the initialise_external() has an optional argument calibrator which has default value -999, if the default value is detected then the self._calibrator is initialized with the self._experiment and a warning is given to the user.
else the self._calibrator is initialized with the calibrator input

Finally:

In all the others default stimuli the _calibrator object is used instead of _experiment.calibrator

new init_external get's also a calibrator object

fedem-p · 2022-03-01T15:48:20Z

stytra/stimulation/__init__.py

-            stimulus.initialise_external(self.experiment)
+            try:
+                stimulus.initialise_external(self.experiment, self.experiment.calibrator)
+            except TypeError:
+                stimulus.initialise_external(self.experiment)
+                warnings.warn("Warning: 'initialise_external' will require a calibrator input from the new update!", FutureWarning)
+                warnings.warn("Warning: 'initialise_external' will require a calibrator input from the new update!", DeprecationWarning)


fedem-p · 2022-03-01T15:49:16Z

stytra/stimulation/stimuli/generic_stimuli.py

+
+        if calibrator == -999:
+            self._calibrator = self._experiment.calibrator
+            warnings.warn("Warning: 'initialise_external' will require a calibrator input from the new update!", FutureWarning)
+            warnings.warn("Warning: 'initialise_external' will require a calibrator input from the new update!", DeprecationWarning)
+        else:
+            self._calibrator = calibrator
+
+


vilim

Good start on the decoupling! I am thinking now with about an alternative implementation would be to pass the ProtocolRunner object that would have the _calibrator (and _estimator_log) parameters (maybe not even directly but under as subfield named something like environment_state that would be a dataclass with calibrator, estimator log and perhaps other things passed from the main process (window dimensions, trigger signal). Then you don't need to stuff many _fields in _stimulus, just a reference to the environment_state that the protocol runner would construct.

stytra/stimulation/stimuli/generic_stimuli.py

and changed calibrator init for stimulus

printout error for feedback; moved initialization of environmentstate var

added class init

fedem-p · 2022-03-08T13:59:12Z

Update:
I have created a Dataclass called EnvironmentState which olds different attributes:

the calibrator
the estimator
height
width

The last two are still unused.

The way it works is the same as before:
the protocol runner passes an _environment_state instance to the stimulus through the initialise_external method.
Here it gets initialized as a stimulus variable (if the input is not an instance of the EnvironmentState class the variable is initialized as a copy of the experiment object)

in the rest of the stimulus files the calls to calibrator and estimator go through the _environment_state variable

fedem-p · 2022-03-08T14:00:30Z

stytra/stimulation/__init__.py

@@ -91,7 +93,9 @@ def __init__(self, experiment=None, target_dt=0, log_print=True):
        self.current_stimulus = None  # current stimulus object
        self.past_stimuli_elapsed = None  # time elapsed in previous stimuli
        self.dynamic_log = None  # dynamic log for stimuli
-
+
+        self.environment_state = EnvironmentState(calibrator = self.experiment.calibrator,)


initialize the env state variable with only the calibrator

fedem-p · 2022-03-08T14:01:04Z

stytra/stimulation/__init__.py

+        if hasattr(self.experiment, 'estimator'):
+            self.environment_state.estimator = self.experiment.estimator


If the experiment class has initialized the estimator pass it to the env_state variable

The more "pythonic" way is try/catch an AttributeError.
In general but this whole logic should be isolated from the ProtocolRunner and handled by either by Experiemnt.get_environment_state(). Also think about how after separating the stimulation in another process, there will be an initialize and update (from queue) methods for the EnvironmentState

fedem-p · 2022-03-08T14:02:31Z

stytra/stimulation/__init__.py

+            try:
+                stimulus.initialise_external(self.experiment, self.environment_state,)
+            except TypeError as e:
+                print("Error: {}".format(e))
+                stimulus.initialise_external(self.experiment)
+                warnings.warn("Warning: 'initialise_external' will use the environment_state variable which holds the calibrator and the estimator object!", FutureWarning)
+                warnings.warn("Warning: 'initialise_external' will use the environment_state variable which holds the calibrator and the estimator object!", DeprecationWarning)


Tries to call the updated function, if it can't it will only pass the experiment object -> further handling is done inside the function

fedem-p · 2022-03-08T14:05:51Z

stytra/stimulation/stimuli/generic_stimuli.py

+@dataclass
+class EnvironmentState:
+    def __init__(self, calibrator = None, estimator = None, height:int = 600, width:int = 800):
+        """
+        Holds Environment variables to pass from the protocol runner to the stimulus
+        """
+        self.calibrator = calibrator
+        self.estimator = estimator
+        self.height = height
+        self.width = width


New dataclass to hold all the variables and objects to pass down to the stimulus

fedem-p · 2022-03-08T14:07:35Z

stytra/stimulation/stimuli/generic_stimuli.py

+        if isinstance(environment_state, EnvironmentState):
+            self._environment_state = environment_state
+        else:
+            self._environment_state = experiment
+            warnings.warn("Warning: 'initialise_external' will use the environment_state variable which holds the calibrator object!", FutureWarning)
+            warnings.warn("Warning: 'initialise_external' will use the environment_state variable which holds the calibrator object!", DeprecationWarning)


If it's an instance of the EnvironmentState class then initialize the variable correctly, otherwise use the new variable as a place holder for the experiment object
(only in place to allow backwards compatibility)

asset_dir, arduino board and logger

access env state var

vilim · 2022-03-19T12:02:27Z

stytra/stimulation/__init__.py

+        if hasattr(self.experiment, 'estimator'):
+            self.environment_state.estimator = self.experiment.estimator


The more "pythonic" way is try/catch an AttributeError.
In general but this whole logic should be isolated from the ProtocolRunner and handled by either by Experiemnt.get_environment_state(). Also think about how after separating the stimulation in another process, there will be an initialize and update (from queue) methods for the EnvironmentState

vilim · 2022-03-19T12:05:18Z

stytra/stimulation/stimuli/generic_stimuli.py

@@ -111,7 +135,7 @@ def stop(self):
        """
        pass

-    def initialise_external(self, experiment):
+    def initialise_external(self, experiment, environment_state: EnvironmentState = None):


I don't think keeping both arguments is a good idea, at some point we anyway need to pass only the environment_state. We can consider having a dev branch to merge such PRs into, and then bump the version to 0.9 after mergining this. Though I don't think custom code relies much on modifying intialise_external (you can check for usage in stytra_config

fedem-p added 3 commits March 1, 2022 14:52

calibrator initialized from the protocol runner;

6fc6347

new init_external get's also a calibrator object

Added warning for deprecated function inputs

4e96d16

Cleaned useless comments

8e79c62

fedem-p commented Mar 1, 2022

View reviewed changes

fedem-p requested a review from vilim March 3, 2022 16:10

fedem-p self-assigned this Mar 3, 2022

fedem-p added the duplicate This issue or pull request already exists label Mar 3, 2022

vilim reviewed Mar 6, 2022

View reviewed changes

stytra/stimulation/stimuli/generic_stimuli.py Outdated Show resolved Hide resolved

fedem-p added 5 commits March 7, 2022 17:46

added EnvironmentState dataclass;

516d893

and changed calibrator init for stimulus

Move data class;

884016a

printout error for feedback; moved initialization of environmentstate var

updated variable name;

e4ffdf0

added class init

Updated calibrator variable

03a99bc

added estimator log variable;

bad8037

fedem-p commented Mar 8, 2022

View reviewed changes

removed useless comments

6db29ec

fedem-p commented Mar 8, 2022

View reviewed changes

fedem-p requested a review from vilim March 9, 2022 09:19

fedem-p added 4 commits March 9, 2022 10:36

change msg for deprecation

7d9cf14

added params to env_state var;

f1b545b

asset_dir, arduino board and logger

updated access to new params;

1f3c8cb

access env state var

Included trigger inside environment state variable

27e377a

vilim reviewed Mar 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean-up Experiment dependencies inside Stimulus #72

Clean-up Experiment dependencies inside Stimulus #72

fedem-p commented Mar 1, 2022 •

edited

Loading

fedem-p Mar 1, 2022

fedem-p Mar 1, 2022

vilim left a comment

fedem-p commented Mar 8, 2022

fedem-p Mar 8, 2022

fedem-p Mar 8, 2022

vilim Mar 19, 2022

fedem-p Mar 8, 2022

fedem-p Mar 8, 2022

fedem-p Mar 8, 2022

vilim Mar 19, 2022

vilim Mar 19, 2022

		if hasattr(self.experiment, 'estimator'):
		self.environment_state.estimator = self.experiment.estimator

Clean-up Experiment dependencies inside Stimulus #72

Are you sure you want to change the base?

Clean-up Experiment dependencies inside Stimulus #72

Conversation

fedem-p commented Mar 1, 2022 • edited Loading

More specifically I expect two cases to throw errors:

To fix 1:

To fix 2:

Finally:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilim left a comment

Choose a reason for hiding this comment

fedem-p commented Mar 8, 2022

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

fedem-p commented Mar 1, 2022 •

edited

Loading