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

Support for custom observation function, minor changes to my previous implementation #126

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

firemankoxd
Copy link
Contributor

Hello Lucas,
This PR contains following changes:

  1. Edit of my previous Implementation (PR-119), just to match the code style.
  2. Support for custom observation function, which can be passed in SumoEnvironment constructor as a Callable or a string. I also added the observation_fns dictionary to the class, as we plan to implement few more observation functions soon. The observation functions can also be registered to this dictionary with the class method.
  3. Similar changes were made also for reward functions (like dictionary with reward functions with registration class method), but most important one was to move the checking of the reward function type to the constructor, because it was checked everytime compute_reward method was called.

@LucasAlegre LucasAlegre merged commit d95389d into LucasAlegre:master Dec 7, 2022
@LucasAlegre
Copy link
Owner

Hi @firemankoxd,

Thanks a lot for this very nice feature! :D

@LucasAlegre
Copy link
Owner

@firemankoxd, I just realized now that if you use a custom observation function, the variable self.observation_space is not updated accordingly. Maybe we should require that a spaces.Box variable is provided when registering a new observation function?

@firemankoxd
Copy link
Contributor Author

firemankoxd commented Dec 7, 2022

Hi @LucasAlegre, thanks for pointing this out! Maybe I could change the observation_fns dictionary to nested one, where every key (e.g. default) would have both observation function and observation space?

observations = {
    'default': {
        'observation_function': x,
        'observation_space': y
    }
}

And as you said, the registration function would require both function and the space.

// Edit: Maybe it would also make sense to create some sort of config/functions file where these functions would be stored, what do you think?

@LucasAlegre
Copy link
Owner

Hi @LucasAlegre, thanks for pointing this out! Maybe I could change the observation_fns dictionary to nested one, where every key (e.g. default) would have both observation function and observation space?

observations = {
    'default': {
        'observation_function': x,
        'observation_space': y
    }
}

And as you said, the registration function would require both function and the space.

That is exactly what I was thinking! :)

// Edit: Maybe it would also make sense to create some sort of config/functions file where these functions would be stored, what do you think?

I agree!

@firemankoxd
Copy link
Contributor Author

Cool! I will try to push the changes by the end of this day, thank you. :)

@firemankoxd
Copy link
Contributor Author

Hey @LucasAlegre,
I talked about what would be the best approach to implement custom observation_space with @michalgregor, and we found out that maybe it would be better to just separate observations to another file and make it as parent class, having both observation function and observation space. All of the additional observation functions together with observation space might be child classes of the default ObservationFunction class. It would also allow us to make the TrafficSignal file/class more readable and clean. However, there are still few things that I need to figure out so if you don't mind I would postpone this implementation, as I need to work on my diploma thesis and do few exams. Meanwhile, you can let me know what do you think about this change. :)

@LucasAlegre
Copy link
Owner

@firemankoxd I think this is a good idea! Good luck with your thesis and exams! We can discuss the best way to implement that later :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants