search

aidudezzz / deepbots

A wrapper framework for Reinforcement Learning in the Webots robot simulator using Python 3.
https://deepbots.readthedocs.io/
GNU General Public License v3.0
230 stars 49 forks source link

Refactored Deepbots: DeepbotsEnv & Robot Class #116

Closed KelvinYang0320 closed 1 year ago

KelvinYang0320 commented 1 year ago

This PR is for:

tsampazk commented 1 year ago

Thanks @KelvinYang0320! Regarding the inheritance from Robot, i remember that when working on it before, there were some issues but i can't recall exactly what. Have you tried using the RobotEmitterReceiver while the class inherits from the Robot? If so, did you notice anything weird?

KelvinYang0320 commented 1 year ago

Have you tried using the RobotEmitterReceiver while the class inherits from the Robot?

@tsampazk Yes, it works fine. I didn't notice anything weird.

Should we also rename 

\deepbots
    \supervisor
        \controllers
            \robot_supervisor.py
            \supervisor_emitter_receiver.py
            \supervisor_env.py

to 

\deepbots
    \deepbots
        \controllers
            \robot_deepbots.py
            \deepbots_emitter_receiver.py
            \deepbots_env.py

or rename supervisor_env.py to deepbots_env.py?

tsampazk commented 1 year ago

Have you tried using the RobotEmitterReceiver while the class inherits from the Robot?

@tsampazk Yes, it works fine. I didn't notice anything weird.

Should we also rename 

\deepbots
    \supervisor
        \controllers
            \robot_supervisor.py
            \supervisor_emitter_receiver.py
            \supervisor_env.py

to 

\deepbots
    \deepbots
        \controllers
            \robot_deepbots.py
            \deepbots_emitter_receiver.py
            \deepbots_env.py

or rename supervisor_env.py to deepbots_env.py?

Ok then! I'll run some tests with the changes and get back to you. I think that we can keep the directory name as supervisor and do the renaming you suggested.

tsampazk commented 1 year ago

I looked into and tested the changes with the new names and inheriting Robot and seem to work well!

To directly answer your question I think that we should keep most names the same apart from supervisor_env.py which should be renamed to deepbots_env.py in my view, as it contains the top class DeepbotsEnv:

\deepbots
    \supervisor
        \controllers
            \robot_supervisor.py
            \supervisor_emitter_receiver.py
            \deepbots_env.py

This makes it clear that the base class which brings webots supervisor and gym env together is the deepbots env which fully conforms with the idea of deepbots as a middleware framework between webots and gym, and shouldn't be used by end-users anyway.

The other classes that inherit DeepbotsEnv are incorporating supervisor duties in practice, in addition to environment duties so it makes sense to keep them named supervisor...

Moving on, I have some additional suggestions:

  1. Add .._env.py at the end of the other files. So new names would be robot_supervisor_env.py and supervisor_emitter_receiver_env.py. I don't really like the long names but i feel it makes sense.
  2. Related to the previous point, rename the classes by adding Env at the end SupervisorEmitterReceiverEnv and SupervisorCSVEnv.
  3. Split SupervisorCSV class in its own file supervisor_emitter_receiver_csv.py or supervisor_csv.py. This would make it similar to the robot classes that are split in robot_emitter_receiver.py and robot_emitter_receiver_csv.py. As of now it is contained in the supervisor_emitter_receiver.py.

What do you think about this? Any other suggestions that might keep the information clearly and make the names shorter is welcome! :smile:

KelvinYang0320 commented 1 year ago

@tsampazk I have a few questions and suggestions:

  1. What is CSV short for?
  2. Could we remove the folder, deepbots/setup?
  3. I think we could let users import RobotEmitterReceiverCSV from deepbots.robots and import RobotSupervisor or SupervisorEmitterReceiverEnv from deepbots.supervisor. Users don't need to understand/see our long filename and be able to run their experiments with deepbots smoothly.
    • Modifying __init__.py.
  1. Add .._env.py at the end of the other files. So new names would be robot_supervisor_env.py and supervisor_emitter_receiver_env.py. I don't really like the long names but i feel it makes sense.

Yes, I agree with you.

  1. Related to the previous point, rename the classes by adding Env at the end SupervisorEmitterReceiverEnv and SupervisorCSVEnv.
  2. Split SupervisorCSV class in its own file supervisor_emitter_receiver_csv.py or supervisor_csv.py. This would make it similar to the robot classes that are split in robot_emitter_receiver.py and robot_emitter_receiver_csv.py. As of now it is contained in the supervisor_emitter_receiver.py.

We already have Env at the end of SupervisorEnv? Do you want to rename it to SupervisorCSVEnv?

tsampazk commented 1 year ago 
1. What is CSV short for?

Comma-separated values. It relates to the way messages are packaged when sent between emitter and receiver.

2. Could we remove the folder, `deepbots/setup`?

I think we can, right now it sits with an empty file inside. This is related to having some kind of setup procedure for deepworlds, installing specific example dependencies etc., which obviously is not implemented yet. :stuck_out_tongue: 

3. I think we could let users import `RobotEmitterReceiverCSV` from `deepbots.robots` and import `RobotSupervisor` or `SupervisorEmitterReceiverEnv` from `deepbots.supervisor`. Users don't need to understand/see our long filename and be able to run their experiments with deepbots smoothly.

   * Modifying `__init__.py`.

Yes exactly, that would be the best solution! Totally agree.

We already have Env at the end of SupervisorEnv? Do you want to rename it to SupervisorCSVEnv?

What the directories/files would like after all of my suggestions as well as the new class names:

\deepbots
    \supervisor
        \controllers
            \deepbots_supervisor_env.py --> contains `DeepbotsSupervisorEnv`
            \robot_supervisor_env.py  --> contains `RobotSupervisorEnv`
            \emitter_receiver_supervisor_env.py  --> contains `EmitterReceiverSupervisorEnv`
            \csv_supervisor_env.py --> contains `CSVSupervisorEnv`

So i switched around the order so the specific type comes in first (emitter-receiver, csv, robot) and all files end with supervisor_env. Moreover, did the same with class names and added Env at the end, and also split csv and emitter receiver in separate files. Following this logic led me to go with DeepbotsSupervisorEnv instead of DeepbotsEnv.

How does this look, taking in mind that we modify the init file so imports are straightforward as you suggested? I feel that having both Supervisor and Env especially in the class names, makes them somewhat long, but keeping only one of them looks like something is missing. On the other hand, we could shorten SupervisorEnv to SE or SEnv or SuperEnv :laughing:, not that i like any of those that much.

KelvinYang0320 commented 1 year ago
  1. What is CSV short for?

Comma-separated values. It relates to the way messages are packaged when sent between emitter and receiver.

Thanks! Got it.

2. Could we remove the folder, `deep bots/setup`?

I think we can, right now it sits with an empty file inside. This is related to having some kind of setup procedure for deepworlds, installing specific example dependencies etc., which obviously is not implemented yet. 😛

Ok, I will remove that folder.

What the directories/files would like after all of my suggestions as well as the new class names:

\deepbots
    \supervisor
        \controllers
            \deepbots_supervisor_env.py --> contains `DeepbotsSupervisorEnv`
            \robot_supervisor_env.py  --> contains `RobotSupervisorEnv`
            \emitter_receiver_supervisor_env.py  --> contains `EmitterReceiverSupervisorEnv`
            \csv_supervisor_env.py --> contains `CSVSupervisorEnv`

So i switched around the order so the specific type comes in first (emitter-receiver, csv, robot) and all files end with supervisor_env. Moreover, did the same with class names and added Env at the end, and also split csv and emitter receiver in separate files. Following this logic led me to go with DeepbotsSupervisorEnv instead of DeepbotsEnv.

How does this look, taking in mind that we modify the init file so imports are straightforward as you suggested? I feel that having both Supervisor and Env especially in the class names, makes them somewhat long, but keeping only one of them looks like something is missing. On the other hand, we could shorten SupervisorEnv to SE or SEnv or SuperEnv 😆, not that i like any of those that much.

@tsampazk It looks great! I think shortening SupervisorEnv to any other name would make users confused. In my opinion, I think we can keep RobotSupervisorEnv and EmitterReceiverSupervisorEnv for now. What do you think?

tsampazk commented 1 year ago

It looks great! I think shortening SupervisorEnv to any other name would make users confused. In my opinion, I think we can keep RobotSupervisorEnv and EmitterReceiverSupervisorEnv for now. What do you think?

Alright, if you are good with these changes, we can go ahead and implement them. I think that this PR is a good place to do it as it already involves refactoring :)

KelvinYang0320 commented 1 year ago

@tsampazk Do we need to rename some class/file names in deepbots/robots/controllers? I have organized the to-do list here.

tsampazk commented 1 year ago

Thank you @KelvinYang0320!

I think that the robot controllers are ok, unless you have some idea of improving them along the other changes.

Changes look good at a first glance, i will test them and get back to you!

KelvinYang0320 commented 1 year ago

@tsampazk I think users would be fine with the naming of RobotSupervisorEnv when using Robot-Supervisor scheme, but the naming of CSVSupervisorEnv and RobotEmitterReceiverCSV seems inconsistent with Emitter-Receiver scheme. I have no idea how to fix that. 😕

Deepbots Usage Robot-Supervisor scheme from deepbots.supervisor import RobotSupervisorEnv Emitter-Receiver scheme from deepbots.supervisor import CSVSupervisorEnv from deepbots.robot import RobotEmitterReceiverCSV

tsampazk commented 1 year ago

You're right @KelvinYang0320, I think that indeed we need to do some renaming in the robot controllers too.

robot_emitter_receiver.py -> emitter_receiver_robot.py, class RobotEmitterReceiver -> EmitterReceiverRobot robot_emitter_receiver_csv.py -> csv_robot.py, class RobotEmitterReceiverCSV -> CSVRobot

I think that these new names make the robot controllers consistent with the corresponding supervisor-env controllers.

Just a reminder that you can also go ahead and remove the setup directory and its contents :smile:

My IDE and also github complain about no newline at end of file for the init files. Moreover, do you think that it would be useful to add all classes in the init files to make them easy to import? The reasoning is that even though we don't expect a user to import and use DeepbotsSupervisorEnv for example, the general concept is that the user is free to do so and implement everything themselves, if they wish, so import should be possible in a similar manner to the "lower" classes.

KelvinYang0320 commented 1 year ago

@tsampazk Hi, there.

robot_emitter_receiver.py -> emitter_receiver_robot.py, class RobotEmitterReceiver -> EmitterReceiverRobot robot_emitter_receiver_csv.py -> csv_robot.py, class RobotEmitterReceiverCSV -> CSVRobot

After renaming, users will use CSVRobot and CSVSupervisorEnv for the Emitter-Receiver scheme, right? I am thinking of "RobotSupervisorEnv" corresponding to the "Robot" "Supervisor" scheme, but no "Emitter" or "Receiver" in the class name, "CSVSupervisorEnv" and "CSVRobot".

Just a reminder that you can also go ahead and remove the setup directory and its contents 😄

Oh, I forget to remove the setup directory. 😆

My IDE and also GitHub complain about no newline at end of the file for the init files.

Thanks for pointing that out. I will fix them.

Moreover, do you think that it would be useful to add all classes in the init files to make them easy to import? The reasoning is that even though we don't expect a user to import and use DeepbotsSupervisorEnv for example, the general concept is that the user is free to do so and implement everything themselves, if they wish, so import should be possible in a similar manner to the "lower" classes.

I think that will be useful! I will add those classes in the init file. I am not sure about what import method is better for keyboard_printer and tensorboard_wrapper.

tsampazk commented 1 year ago

After renaming, users will use CSVRobot and CSVSupervisorEnv for the Emitter-Receiver scheme, right?

Yes!

I am thinking of "RobotSupervisorEnv" corresponding to the "Robot" "Supervisor" scheme, but no "Emitter" or "Receiver" in the class name, "CSVSupervisorEnv" and "CSVRobot".

You're right about that but the names CSVEmitterReceiverSupervisorEnv and CSVEmitterReceiverRobot, seem a little bit long :laughing:. Also, a user taking a look at the csv class can see it inheriting from the emitter receiver class and i think there are a lot of explanations in the tutorials and readmes. I don't have a strong opinion on this to be honest, i can see both working for different reasons. :confused:

I think that will be useful! I will add those classes in the init file. I am not sure about what import method is better for keyboard_printer and tensorboard_wrapper.

* `from deepbots.supervisor.wrappers import KeyboardPrinter`, `from deepbots.supervisor import KeyboardPrinter`, or `from deepbots import KeyboardPrinter`?

I think the first one is ok, as the wrappers are extra utilities that can be used!

KelvinYang0320 commented 1 year ago

I am thinking of "RobotSupervisorEnv" corresponding to the "Robot" "Supervisor" scheme, but no "Emitter" or "Receiver" in the class name, "CSVSupervisorEnv" and "CSVRobot".

You're right about that but the names CSVEmitterReceiverSupervisorEnv and CSVEmitterReceiverRobot, seem a little bit long 😆. Also, a user taking a look at the csv class can see it inheriting from the emitter receiver class and i think there are a lot of explanations in the tutorials and readmes. I don't have a strong opinion on this to be honest, i can see both working for different reasons. 😕

I get you. We can keep CSVRobot and CSVSupervisorEnv. 😃

I think that will be useful! I will add those classes in the init file. I am not sure about what import method is better for keyboard_printer and tensorboard_wrapper.

* `from deepbots.supervisor.wrappers import KeyboardPrinter`, `from deepbots.supervisor import KeyboardPrinter`, or `from deepbots import KeyboardPrinter`?

I think the first one is ok, as the wrappers are extra utilities that can be used!

Okay!

@tsampazk I have updated the PR. Could you review the code and deepworlds again? I have organized the to-do list https://github.com/aidudezzz/deepbots/pull/116#issue-1423367164.

As for snake_case function naming, I think we can discuss it in another PR (or issue)? Also, I notice that there are some warning messages when I use Pylint to check deepbots.

tsampazk commented 1 year ago

Thanks @KelvinYang0320, i think we are in a good spot now, i will review the changes as a whole and get back to you.

As for snake_case function naming, I think we can discuss it in another PR (or issue)? Also, I notice that there are some warning messages when I use Pylint to check deepbots.

You mean warnings regarding snake_case naming etc.? I think we need to take care of this in all repos (deepbots, deepworlds especially, and tutorials), as we discussed in the past. I will open a relevant issues on all repos.

KelvinYang0320 commented 1 year ago

You mean warnings regarding snake_case naming etc.?

Most of the warnings are about snake_case naming.

I think we need to take care of this in all repos (deepbots, deepworlds especially, and tutorials), as we discussed in the past. I will open a relevant issues on all repos.

Sounds good! We can keep track of the naming issue on all repos.

i will review the changes as a whole and get back to you.

@tsampazk Thank you 😄

tsampazk commented 1 year ago

Sorry @KelvinYang0320 i once again used the update branch button without using the rebase option. I reverted the changes and updated with rebase. :sweat: Should be good now as far as i can tell, please double-check to make sure.

I am in the process of implementing a new example for deepworlds, using the refactor deepbots, so i can see it in action and identify any problems.