search

dfki-ric-underactuated-lab / double_pendulum

Dual purpose Acrobot and Pendubot Platform
BSD 3-Clause "New" or "Revised" License
33 stars 22 forks source link

Submission for EvolSAC #15

Closed AlbertoSinigaglia closed 2 weeks ago

fwiebe commented 3 weeks ago

Thanks for the pull request :)

Could you do the following changes before, I merge?

  1. Could you put the Dockerfile in a folder called docker and call it Dockerfile_evolsac? So that the main Dockerfile will not be overwrtitten. We will collect the Dockerfiles for different controllers then in the docker folder.

  2. Could you undo the changes in the following files (I think they are not related to your controller:

    • examples/realistic/saclqr.py
    • leaderboard/acrobot/robustness_v1/con_sac_lqr.py
    • leaderboard/acrobot/simulation_v1/con_sac_lqr.py
    • leaderboard/pendubot/robustness_v1/con_sac_lqr.py
    • leaderboard/pendubot/simulation_v1/con_sac_lqr.py

  3. Could you remove the following files ( I think you moved them to the data folder):

    • leaderboard/acrobot/model.zip
    • leaderboard/pendubot/model.zip

  4. Could you also undo the changes in the sim_parameters.py files? If you want to use your controller with a lower torque limit or have other controller specific limits you can define that in the file which instantiates the controller, i.e. the con_evolsac.py files.

    • leaderboard/acrobot/robustness_v2/sim_parameters.py
    • leaderboard/acrobot/simulation_v2/sim_parameters.py
    • leaderboard/pendubot/robustness_v2/sim_parameters.py
    • leaderboard/pendubot/simulation_v2/sim_parameters.py

  5. I think, for now it is ok if you fix the stable_baselines version to 2.3.2 in the setup.py file. If a newer version is needed in the future, I will look into how to convert policies between stable_baseline versions. Are the other dependencies compatible with the existing ones? Or is there another conflict? Is the specific numpy version 1.24.1 required?

The rest looks good. Let me know If you have questions or if I some of my requests would break something for your contribution.

fwiebe commented 2 weeks ago

Hi @AlbertoSinigaglia did you address the remarks from above? There are two conflicts that must be resolved before merging. One related to the original Dockerfile which is supposed to stay in the root folder for now and one related to the src/python/setup.py file, which should be easily resolvable. Let me know when you finished editing and I will merge :)

MarcoCali0 commented 2 weeks ago

Hi @fwiebe, I've just restored the root Dockerfile and addressed the src/python/setup.py differences. Also fixed the typos on the following commit.

fwiebe commented 2 weeks ago

@MarcoCali0 thanks! I will merge it now :)