Conversation
sergiovalmac
left a comment
sergiovalmac
left a comment
There was a problem hiding this comment.
Very exciting getting the new policy in place!
We have discussed most of my major comments during the peer-review session. I have added other relevant comments like summarising snippets of code into functions and renaming some variable to enhance readability. Plus some minor comments, mainly missing/outdated docstrings, missing commas, etc.
| load_ph, sigma_2_ph = load_sig | ||
| return load_ph, sigma_2_ph | ||
| load_ph, sigma_2_ph, w_dirs_to_resources = load_sig | ||
| return load_ph, sigma_2_ph, w_dirs_to_resources |
There was a problem hiding this comment.
Please correct the return type annotation, and extend the docstring to account for the new variable in the description and to include :return field.
| """ | ||
| strategic_idling_tuple = self.strategic_idling_object.get_allowed_idling_directions(state) | ||
| strategic_idling_tuple = self.strategic_idling_object.get_allowed_idling_directions(state, | ||
| safety_stocks_vec) |
There was a problem hiding this comment.
Is passing safety_stocks_vec here general and valid for any strategic idling class? or is it specific of the foresight variant and will break if we use any other class?
| #kwargs_get_policy = self.serialise_get_policy_kwargs(**kwargs) | ||
| #z_star, _ = self.policy_obj.get_policy(**kwargs_get_policy) |
There was a problem hiding this comment.
Aren't we breaking the code here?
Does this work with any other class different from FoxMpc?
|
|
||
| # If any resource is starving or countdown ends, then recompute activity rates. | ||
| if self.num_steps_to_recompute_policy == 0: | ||
| if True:#self.num_steps_to_recompute_policy == 0: |
There was a problem hiding this comment.
Is this a hack for bypassing a json config file?
| #self.num_steps_to_recompute_policy = self.get_num_steps_to_recompute_policy( | ||
| #current_horizon, | ||
| #self.hedgehog_hyperparams.horizon_mpc_ratio, | ||
| #self.hedgehog_hyperparams.minimum_horizon | ||
| #) |
There was a problem hiding this comment.
Why bypassing all this? Will this work beyond FoxMpc?
| state=state, | ||
| x_star = state, | ||
| x_eff = state, | ||
| r_idling_set = np.array([]), | ||
| draining_resources = set(), |
There was a problem hiding this comment.
Is this really needed or a hack to use a different strategic idling class by default?
There was a problem hiding this comment.
simulation to estimate asymptotic covariance uses the same mpc policy as the real agent but passes a different fluid policy. In case of Fox instead of fluid policy, effective state should be passed to mpc policy. how can we work around it here?
| :return: set of allowed idling resources with auxiliary variables | ||
| """ | ||
| w = self._workload_mat @ state | ||
| self._safety_stocks_vec = safety_stocks_vec |
There was a problem hiding this comment.
This seems out of nowhere here and difficult to know when it will be used. Can we do anything about that?
| self._compute_num_roll_out_steps() | ||
|
|
||
| def get_allowed_idling_directions(self, state: StateSpace) -> StrategicIdlingOutput: | ||
| def get_allowed_idling_directions(self, state: StateSpace, safety_stocks_vec) -> StrategicIdlingOutput: |
There was a problem hiding this comment.
This seems out of nowhere here and difficult to know when it will be used. Can we do anything about that? Happy to discuss...
| self._cost_per_buffer.T @ x_var | ||
| + penalty_coeff_w_star * cvx.sum(self._workload_mat @ x_var - w_par)) | ||
| constraints = [self._workload_mat @ x_var >= w_par] | ||
| a_mat = np.vstack(self.list_boundary_constraint_matrices) |
There was a problem hiding this comment.
Could we rename a_mat more descriptively, maybe something like resource_to_buffer_mat?
|
|
||
| def _find_workload_with_min_eff_cost_by_idling(self, w: WorkloadSpace) -> WorkloadSpace: | ||
| self._w_param.value = w | ||
| self._safety_stocks_param.value = np.zeros_like(self._safety_stocks_vec) |
There was a problem hiding this comment.
Do we need this initialisation if we set it to the right value below in Line 269?
Co-authored-by: sergiovalmac <serteckian@gmail.com>
Co-authored-by: sergiovalmac <serteckian@gmail.com>
2 participants
Prototype of the reactive effective state pressure MPC policy