Open middleyuan opened 2 months ago
Looks much cleaner! Nice! I think the samplers could still be made a little more flexible? Let me know if you think this is feasible.
Looks pretty good! I have a couple thoughts though:
- It seems like there is a lot of controller-specific code. For example, there is a separate sampler for each controller, and in the HPO code, there are if statements depending on the algorithm being optimized. I'm wondering if there is a way to make this more generic by making an HPO yaml more complex and then having the underlying code make classes from the arguments in these yamls? For example, I think that hpo_sampler.py could almost entirely be defined in a yaml and then having a generic class for sampler that parses the yaml appropriately? It feels like there is a lot of repeated code that could be simplified and the addition of future algos simpler?
- the HPO class is defined in both hpo_optuna.py and hpo_vizier.py which seem to share a lot of code. I think there should really be a parent HPO class, and then child sublcasses for the different use cases?
- I'm not totally sure why the files are being removed from the examples. Is it because you are replacing them with better hyper parameters?
General comments: to get HPO module fully tested, I am waiting for another PR (quadrotor interface) to be approved. After that I will run unit-test for HPO on new env interface and also run pre-commits hook.
This PR is created for two primary purposes: