Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Noise in the environment 🐛 #385

Closed
riccardopoiani opened this issue Aug 11, 2021 · 5 comments
Closed

Noise in the environment 🐛 #385

riccardopoiani opened this issue Aug 11, 2021 · 5 comments
Labels
🐛 bug Something isn't working.

Comments

@riccardopoiani
Copy link

riccardopoiani commented Aug 11, 2021

Description

Currently, the data generation process works in the following way.
Whenever there is noise in the environment configurations, fixed a seed, the data generation process will be fixed, thus generating always the same "environment trajectory" (i.e., same order distribution, same vessel speeds, same vessel parking noise).

Expected Behavior

I would expect each "environment trajectory" to be different from the previous one (i.e., after a reset), in the sense that a different noise should be applied each time the environment is reset.

This is crucial also for the different reasons that are mentioned in both of your papers: if not done, the environment is fully deterministic (and one of the main reason to apply methods based on RL is the way in which they can handle uncertainty, as it happens in truly real scenarios indeed).
If this is not done, the performances that any RL-based method is able to achieve are flawed. In this case, indeed, it is obvious that the method is overfitting the "noise" in that specific configuration (at this point, it is even missleading to call it noise, since each trajectory generates the same exact data) .

Environment

  • MARO version (e.g., v0.1.1a1): master
  • MARO scenario (CIM, Citi Bike): CIM
  • MARO component (Simulation, RL, Distributed Training): Simulation
  • Orchestration platform (GraSS on Azure, AKS on Azure):
  • How you installed MARO (pip, source): source
  • OS (Linux, Windows, macOS): Linux
  • Python version (3.6, 3.7): 3.7
  • Docker image (e.g., maro2020/maro:latest):
  • CPU/GPU:
  • Any other relevant information:
@riccardopoiani riccardopoiani added the 🐛 bug Something isn't working. label Aug 11, 2021
@riccardopoiani
Copy link
Author

riccardopoiani commented Aug 11, 2021

The easiest way that I've found to achieve the reset is the following one.
Modify cim_data_container_helpers/CimDataContainerWrapper.reset() in the following way:

def reset(self):
    """Reset data container internal state"""
    #self._data_cntr.reset()
    self._init_data_container()

Then, in cim_data_generator/CimdDataGenerator.gen_data()

#topology_seed = conf["seed"]
from random import randint
topology_seed = randint(0, 4096)

Probably there are cleaner ways, but I find the current seed mechanism to be too complicated for me to be analysed so I'm leaving this fix here for whoever is interested in a shortcut for fixing this problem.

P.S. It has not been tested with Real Data mode

@Jinyu-W
Copy link
Collaborator

Jinyu-W commented Aug 12, 2021

Thank you @riccardopoiani ! It is really a good comment and suggestion. We'll find some way to handle this problem.

@lihuoran lihuoran mentioned this issue Aug 19, 2021
20 tasks
@lihuoran
Copy link
Contributor

Hi @riccardopoiani , on the latest master branch, we add a new parameter keep_seed to Env.reset(). if keep_seed == True, then all the random seeds will be reset to the initial values, hence you should get same random outputs in the next episode. If this is not what you prefer, you can set keep_seed to False to get new random outputs. Under this situation, the vessels' plan will be re-generated and other random events (for example, order generation) will be different with the last episodes.

The default value of keep_seed is False now, which means keeping randomness is the default option.

Thanks for your contribution! Please have a try.

@riccardopoiani
Copy link
Author

riccardopoiani commented Aug 20, 2021

Thanks @lihuoran! it seems to be working to me. However, personally I would also add something like this to cim_data_generator.py:

if topology_seed is None:
    if conf["seed"] is None:
        from random import randint
        topology_seed = randint(0, 4096)
    else:
        topology_seed = conf["seed"]

In this way, it is also possible to have an environment that starts with different trajectory at each run.
The reason why I'm asking for this feature is the following.
I have algorithms that works with a single trajectory. Without the possibility of setting the seed to null in the configuration file, each run will have the same environmental trajectory and I should, every time, set a different seed by hand.

@lihuoran
Copy link
Contributor

Thanks @lihuoran! it seems to be working to me. However, personally I would also add something like this to cim_data_generator.py:

if topology_seed is None:
    if conf["seed"] is None:
        from random import randint
        topology_seed = randint(0, 4096)
    else:
        topology_seed = conf["seed"]

In this way, it is also possible to have an environment that starts with different trajectory at each run.
The reason why I'm asking for this feature is the following.
I have algorithms that works with a single trajectory. Without the possibility of setting the seed to null in the configuration file, each run will have the same environmental trajectory and I should, every time, set a different seed by hand.

We will discuss your idea in the future. For now, you could customize the code on your local branch according to your needs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working.
Projects
None yet
Development

No branches or pull requests

3 participants