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

Create Trajectory class #52

Closed
bmielnicki opened this issue Aug 19, 2020 · 2 comments
Closed

Create Trajectory class #52

bmielnicki opened this issue Aug 19, 2020 · 2 comments
Labels
Refactor Improve codebase without modifying functionality

Comments

@bmielnicki
Copy link
Collaborator

Creating a Trajectory class to better abstract away a lot of the complex dictionary parsing in a more Object Oriented fashion.
Moved from #44 as this issue will probably wait a bit.

@bmielnicki bmielnicki added the Refactor Improve codebase without modifying functionality label Aug 19, 2020
@bmielnicki bmielnicki self-assigned this Sep 25, 2020
@bmielnicki
Copy link
Collaborator Author

I have a couple of questions before starting making code here:

  1. Do we want this task done considering it was initially part of Unify OvercookedGridworld and OvercookedEnv classes #44 that probably won't happen (current discussion ended on the decision to not do this issue)?

  2. How much do we care about fully backward compatibility here? How I can solve backward compatibility:
    a) Implement a method that returns trajectory dict in its current form
    b) Implement magic methods like __getitem__ to let users use trajectory object exactly like trajectory dict. It is a bit constraining (at least prevent from use magic method in the way different that was implemented in dict) and there is a risk of forgetting some magic method.
    c) just don't care about it - edit overcooked-ai and harl repo to work with the changes and that it enough
    d) some mix of above

  3. Should I create it from the master branch or trajectory visualization PR code Trajectory visualization #38 (it depends if anyone wants this change before the merge of Trajectory visualization #38 will happen - currently it waits for review after some refactor and added compatibility with the adjectives)?

@micahcarroll
Copy link
Member

micahcarroll commented Sep 25, 2020

@nathan-miller23 & @mesutyang97, I think this is kind of up to you (whether we should do this now or not) – I wouldn't expect either of you to be using this version of the code anytime soon (unless you think it valuable enough to spend time merging it with yours). Moreover, I expect this to require quite a large overhaul of human_aware_rl and other repos, such as your own project ones – so even reviewing this PR and the corresponding ones in other repos (or making those yourselves) would likely be a lot of work.

I personally think that it's probably not worth it right now, and we should punt it to a better time in which there is less pressure on getting actual research done.

@bmielnicki bmielnicki removed their assignment Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Improve codebase without modifying functionality
Projects
None yet
Development

No branches or pull requests

2 participants