Refactor/pure functions #1944
Refactor/pure functions #1944
Conversation
|
This pull request introduces 16 alerts and fixes 17 when merging c5ee9e2 into aabefef - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert when merging c5ee9e2 into 11ccd54 - view on LGTM.com new alerts:
|
| modulable_params = {p.name: _get_param(p) for p in self.parameters} | ||
| if params is None: | ||
| params = {} | ||
| parameters = {**modulable_params, **params} |
There was a problem hiding this comment.
| parameters = {**modulable_params, **params} | |
| parameters = {**modulable_params, **kwargs, **params} |
There was a problem hiding this comment.
Adding kwargs here prevents multiple keyword arguments error on ex: pnl.Linear()(variable=5, slope=10, intercept=1)
| return self._derivative(**kwargs, **parameters) | ||
| else: | ||
| value = self._function(**kwargs, **parameters) |
There was a problem hiding this comment.
| return self._derivative(**kwargs, **parameters) | |
| else: | |
| value = self._function(**kwargs, **parameters) | |
| return self._derivative(**parameters) | |
| else: | |
| value = self._function(**parameters) |
| def _get_param(p): | ||
| try: | ||
| return self._get_current_parameter_value(p.name, context) | ||
| except: |
There was a problem hiding this comment.
Should catch specific exception[s], AttributeError probably
| return self._get_current_parameter_value(p.name, context) | ||
| except: | ||
| print(e) | ||
| modulable_params = {p.name: _get_param(p) for p in self.parameters} |
There was a problem hiding this comment.
Depending on what's faster, could only call _get_param on parameters in the function signature (see caching in prune_unused_args
PsyNeuLink/psyneulink/core/globals/utilities.py
Lines 1611 to 1615 in a2d06bf
Should also remove need for dummy args like in L411 **_): and unused args like L2454:L2456
| x_0=None, | ||
| offset=None, | ||
| scale=None, | ||
| **_): |
There was a problem hiding this comment.
Still _get_current_parameter_value calls inside
| input = np.asarray(input).copy() | ||
| input[input>0] = gain | ||
| input[input<=0] = gain * leak |
There was a problem hiding this comment.
| input = np.asarray(input).copy() | |
| input[input>0] = gain | |
| input[input<=0] = gain * leak | |
| variable = np.asarray(variable).copy() | |
| variable[variable>0] = gain | |
| variable[variable<=0] = gain * leak |
also line 1550
return variable
|
Other than the few design issues. This is missign two major design justifications: 1.) Can all 2.) How is the new design going to be enforced? Is every new Function expected to conform to the new format? If so what kind of help/guidance/enforcement will there be to guarantee that? Will there be a need for similar cleanup every time few more Functions are added? |
| def _derivative(variable=None, | ||
| slope=None, | ||
| **_): | ||
| return np.reshape(slope, variable.shape) |
There was a problem hiding this comment.
this should use broadcast instead of reshape. reshape needs compatible shapes (same number of elements)
| def _function(variable=None, | ||
| slope=None, | ||
| intercept=None, | ||
| **_): |
There was a problem hiding this comment.
This should update the argument documentation
| def _function(variable=None, | ||
| slope=None, | ||
| intercept=None, | ||
| **_): |
There was a problem hiding this comment.
Why do you need hidden kwargs?
| if params is None: | ||
| params = {} | ||
| parameters = {**modulable_params, **params} | ||
| parameters["variable"] = variable |
There was a problem hiding this comment.
shouldn't this update the variable param in the current context?
| if derivative: | ||
| return self._derivative(**kwargs, **parameters) | ||
| else: | ||
| value = self._function(**kwargs, **parameters) |
There was a problem hiding this comment.
This is a rather ugly extra redirection. why can't you just move the checks to a helper method and call that from both function and derivative?
|
|
||
| def derivative(self, *args, **kwargs): | ||
| return self.function(derivative=True, *args, **kwargs) |
There was a problem hiding this comment.
This should be:
params = self._process_params(*args, **kwargs)
return self._derivative(params)
It should also allow you to drop the **_ ugliness.
@kmantel
@jvesely
this is a draft of the pure transferfunction refactor - I could definitely use some input on if this is a good approach, or any other suggestions.
autocompiled udf code isn't in this draft yet - i want to completely nail down pure functions first