Skip to content
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

Clean-up Experiment dependencies inside Stimulus #72

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Conversation

fedem-p
Copy link
Member

@fedem-p fedem-p commented Mar 1, 2022

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:

  1. if the protocol runner calls a custom stimulus where the initialise_external() function doesn't accept the calibrator input.
  2. 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

Comment on lines 115 to 121
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIX 1

Comment on lines 135 to 143

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


Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix 2

@fedem-p fedem-p requested a review from vilim March 3, 2022 16:10
@fedem-p fedem-p self-assigned this Mar 3, 2022
@fedem-p fedem-p added the duplicate This issue or pull request already exists label Mar 3, 2022
Copy link
Member

@vilim vilim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
and changed calibrator init for stimulus
printout error for feedback;
moved initialization of environmentstate var
added class init
@fedem-p
Copy link
Member Author

fedem-p commented Mar 8, 2022

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

@@ -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,)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize the env state variable with only the calibrator

Comment on lines +117 to +118
if hasattr(self.experiment, 'estimator'):
self.environment_state.estimator = self.experiment.estimator
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 122 to 128
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 6 to 15
@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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 147 to 152
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@fedem-p fedem-p requested a review from vilim March 9, 2022 09:19
Comment on lines +117 to +118
if hasattr(self.experiment, 'estimator'):
self.environment_state.estimator = self.experiment.estimator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

def initialise_external(self, experiment):
def initialise_external(self, experiment, environment_state: EnvironmentState = None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants