Adding updates for Quadrotor #166
Conversation
…ckpoint in ppo.py. 4. Boolean var in ppo_sampler.
…izier. 3. improve parallel computes for Vizier.
… logging. 3. change PPO search space. 4. make cost parameters independent. 5. save intermediate results during HPO. 6. two packages tested on iLQR, GPMPC, and PPO.
Federico-PizarroBejarano
requested review from
Federico-PizarroBejarano and
adamhall
There was a problem hiding this comment.
It looks good but I am having trouble understanding what is an addition and what is a bug fix. It seems like a lot of the changes have impacts on the other environments. Thus, I have two main comments:
- I would like to better understand how these changes affect the other environments, with some guarantee that they either fix or do not change their behaviour
- I think you've made a lot of changes to the pre-process function. I am not sure why but I think we need to standardize how it is used.
Additionally, in terms of linting, you need to run the pre-commit hooks. We should all install them so that they all run on every commit. Additionally, you have commented out a lot of code. For merging to main I think there should be essentially no commented out code: either the code is useful and integrated or it is obselete and deleted. Happy to discuss edge-cases
| processed_action = self._preprocess_control(action) | ||
| return processed_action | ||
| # processed_action = self._preprocess_control(action) | ||
| return action |
There was a problem hiding this comment.
This seems like a serious breaking change that would dramatically change the behaviour of the other environments. Am I misunderstanding something?
There was a problem hiding this comment.
Yes, this will change the behaviour. The primary necessity was that the attitude controller needs to run at a higher frequency and is made to be a part of the preprocess function. Then, the intent is to run "preprocess_action" at the same frequency as pyb in advance_simulation.
Corresponding care is taken to correct it in cartpole.
There was a problem hiding this comment.
I am not sure if y'all discussed this yet but we should talk about the best way to change the preprocess function to accomodate the attitude controller then. I don't understand what the impacts of this change are yet
| Check the only line in this method and `_update_and_store_kinematic_information()` | ||
| to understand its format. | ||
| ''' | ||
| state = np.hstack([ | ||
| self.pos[nth_drone, :], self.quat[nth_drone, :], | ||
| self.rpy[nth_drone, :], self.vel[nth_drone, :], | ||
| self.ang_v[nth_drone, :], self.last_clipped_action[nth_drone, :] | ||
| self.ang_v[nth_drone, :], self.rpy_rates[nth_drone, :], self.last_clipped_action[nth_drone, :] |
There was a problem hiding this comment.
what is the difference? does this affect the other systems? was it a bug beforehand?
| pos = pos + self.PYB_TIMESTEP * vel | ||
| rpy = rpy + self.PYB_TIMESTEP * rpy_rates | ||
| # Set PyBullet's state. | ||
| p.resetBasePositionAndOrientation(self.DRONE_IDS[nth_drone], |
There was a problem hiding this comment.
the drone state is set in several places. Didn't you create a function for this? Could it be used to decrease this repeated code?
There was a problem hiding this comment.
That's from the original code. Not sure, why it got highlighted.
There was a problem hiding this comment.
Fair enough. It seems you have added a function that can be used to clean up the code a lot tho, would be nice to replace all those chunks with your new _set_pybullet_information function.
Changes: