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

Trajectory visualization #38

Conversation

bmielnicki
Copy link
Collaborator

@bmielnicki bmielnicki commented Aug 4, 2020

Changes are mostly for method events_visualization in AgentEvaluator from overcooked_ai_py/agent/benchmarking.py
Example look:
chart-with-legends

@staticmethod
def events_visualization(trajs, traj_index=0, ipython=False, chart_settings=None):
    """
    Displays chart with visualization of events (when items pickups happened etc.)
    ipython - chooses between opening chart
    in default browser or below ipython cell (for ipython=True)
    chart_settings - json with various chart settings that overwrittes default ones
    for more info see create_chart_html function with comments aboutr default chart settings
    """

More on chart settings and the way defaults are updated:

def create_chart_html(events_data, chart_settings=None):
    # all numerical values are pixels
    # not explicity stated below margins are not implemented
    default_chart_settings = {'height': 250, # height of whole chart container
        'width': 720,  # width of whole chart container
        'margin': {'top': 20, 'right': 60, 'bottom': 180, 'left': 40}, # margins of chart (not container) without legends
        'hold_line_width': 3, # hold line is line between pickup and drop event that symbolizes holding item by the player
        'highlighted_hold_line_width': 6, # highlighting element is by hovering mouse over something associated with described object
        'object_line_width': 0, 'highlighted_object_line_width':3,
        'object_event_height': 10, 'object_event_width':16, # object event is triangle
        'label_text_shift': 30, # how far from axis is axis label
        'add_cumulative_data': True, # cumulative data is data about cumulative events
        'cumulative_data_ticks': 4,
        'show_legends': True, # when setting show_legends to False it is recommended to also lower height and margin.bottom
        'legend_title_size': 10, 'legend_points_height': 10, 'legend_points_width': 16, 
        'legend_points_margin': {'bottom': 5, 'right': 5}, # margin after legend point and next point or legend text
        'legend_margin': {'right':  5} # margin between legend columns
        }

    settings = default_chart_settings.copy()
    settings.update(chart_settings or {})

Example usage looks mostly like that:

from overcooked_ai_py.agents.benchmarking import AgentEvaluator
agent_eval = AgentEvaluator({"layout_name": "cramped_room"}, {"horizon": 100})
trajectory_human_pair = agent_eval.evaluate_human_model_pair(num_games=1, display=False)
agent_eval.events_visualization(trajectory_human_pair, traj_index=0)

There is also small tutorial notebook I wrote: overcooked_ai/overcooked_ai_py/tutorial_notebooks/Agent_evaluator_intoduction.ipynb .

Some of the things I'm not sure about:

  • Changing items (e.g. filling dish with soup) is treated as both pickup and drop (drop dish, pickup soup) what increments cumulative stats by 2.
  • I put all helper functions that won't be probably used outside extract_events inside the function. I can move them outside of the function (especially bigger ones), but I do not want to suggest they have use outside of the function.
  • The function of extraction events is adding ids to object as post-processing of trajectory (to highlight single object events feature). While now this works perfectly its less robust to modifications (e.g. code is assuming that object on the ground won't move without a player, filling dish with soup creates new object_id for soup instead of taking object_id of the dish) than putting ids of objects in MDP files so it is temporary. I can add object ids to original objects when I will change MDP files.
  • Using d3.js and require.js from URL, so it won't work offline. I wanted to do it in a more elegant way (from an offline package), but after a couple of failed tries I gave up (I do not want to spend too much time on this detail if it's not important).
  • All CSS and js code used in the chart is copied straight into HTML - this ensures HTML with chart will always work (e.g. will be no CORS errors), on the other hand, this adds 7 KiB to every HTML with the chart and looks less elegant than importing files.
  • I upped canvas version to 2.6 because installation of canvas 2.5 throw errors (tries to download a file that does not exist, this is the problem with npm package).
  • I've moved all visualization code to another folder (accessible from the AgentEvaluator object method) because we probably want to avoid the object of size 1k + lines. I'm not sure if this folder should exist in agents folder or directly below overcooked_ai_py.

@bmielnicki bmielnicki marked this pull request as draft August 4, 2020 19:18
@micahcarroll
Copy link
Member

Hi Bartek, thanks for you PR!

Just as a quick note, it would generally be useful if you add an example usage of the new functionality you introduce in your PRs descriptions (if it's not obvious), and in the case of visualizations also add an example jpeg!

In any case, I do think that it seems to me like many of the changes here are not necessarily compatible in philosophy with the updated repo, so we might just want to leave these changed pending and not merge this PR (I'm sure at a later time, some of this code could be salvaged). In particular, we have now moved to a purely-python implementation of overcooked_ai (using the Flask app in overcooked-demo, which has only the most essential js code) – @nathan-miller23 (who set this new system up) can correct me if I'm saying something wrong here. This seems at odds with having the visualizations depend on a js module, instead of using purely python-based visualization methods (such as matplotlib). However, I haven't looked too in depth into your changes, so I might also be missing something.

Also, not sure if you saw that this branch is out-of-date with the base branch, so in any case it would require updating before it would even be possible to merge.

@bmielnicki
Copy link
Collaborator Author

Hi Bartek, thanks for you PR!

Just as a quick note, it would generally be useful if you add an example usage of the new functionality you introduce in your PRs descriptions (if it's not obvious), and in the case of visualizations also add an example jpeg!

In any case, I do think that it seems to me like many of the changes here are not necessarily compatible in philosophy with the updated repo, so we might just want to leave these changed pending and not merge this PR (I'm sure at a later time, some of this code could be salvaged). In particular, we have now moved to a purely-python implementation of overcooked_ai (using the Flask app in overcooked-demo, which has only the most essential js code) – @nathan-miller23 (who set this new system up) can correct me if I'm saying something wrong here. This seems at odds with having the visualizations depend on a js module, instead of using purely python-based visualization methods (such as matplotlib). However, I haven't looked too in depth into your changes, so I might also be missing something.

Also, not sure if you saw that this branch is out-of-date with the base branch, so in any case it would require updating before it would even be possible to merge.

I think both pure python and python+js approaches for visualizations are ok depending on the interactivity level we want to have.

Pros of having charts in js runnable from python:

  • easy to make interactive charts (now used only for highlighting events associated with a single item, this alone probably does not justify adding js)
  • easy to add another js code like a graphic-based version of AgentEvaluator.interactive_from_traj in the same HTML

Cons of having charts in js:

  • current charting code requires both python (pre-processing trajectory data) and javascript (representation)
  • need some workarounds to work in a web browser (like saving HTML in /tmp directory to open it from file)

Additional questions:

  1. Do we want to add a graphic-based version of AgentEvaluator.interactive_from_traj? If yes, do we want (and how) use code from overcooked-demo/server/graphics/overcooked_graphics_v2.1.js ?
  2. Should I refactor code now (migrating some code to MDP from post-processing of trajectory, maybe migrating js code to pure python if it will be confirmed as the way to go) or wait with it? It mostly depends if we want to merge it in the near future to master (if not I think it's better to wait with it, many stuff can change in the meantime).

Copy link
Collaborator

@nathan-miller23 nathan-miller23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Bartek,

Sorry for the long delay. In general I really like your code! Very well documented and easy to follow with lots of intuitive helper functions. That helps A LOT and makes this whole process easier for everyone involved so thanks!

As far as the dependency on JS code, I think that's actually ok in this case, despite the consistent effort to migrate entirely to python. I will note however, that any JS code you write should be done sparingly and with caution, with the following requisites:

  • Able to be easily invoked from a python function
  • Able to easily return any data/computation/results back to the python function
  • Does the absolute bare minimum of pre-processing/processing. This one is the most important; there should be absolutely ZERO overcooked logic in any JS code

However, it does appear that your code here abides by all of these constraints, so I think it would be ok to incorporate it. Going forward, however, I highly recommend using JS only as a last resort, only after convincing yourself its infeasible to do it natively in python.

overcooked_ai_py/visualization/extract_events.py Outdated Show resolved Hide resolved
overcooked_ai_js/package.json Outdated Show resolved Hide resolved
overcooked_ai_py/agents/benchmarking.py Outdated Show resolved Hide resolved
@bmielnicki bmielnicki marked this pull request as ready for review August 13, 2020 06:41
@bmielnicki bmielnicki marked this pull request as draft August 13, 2020 07:10
@bmielnicki
Copy link
Collaborator Author

bmielnicki commented Aug 19, 2020

Let me know if we want to change objects_ids to deterministic (e.g. ints counting from 0, use of seeded random functions) so we can use them in eq functions.

I definitely think we need to include the object_id in the __eq__ method, at least that seems like the more intuitive way to say two objects are "equal". I would, however, include a utility function, like uid_independent_equal or something, that checks if two ObjectStates are equal in all areas except ID. This function can then be used for tests (sorta like how GridworldState has a timestep_independent_equal function). Another (perhaps cleaner) way to do it is to have the uid come from the numpy random suite and then set np.random.seed as you suggested.

Both of these approaches seem equally viable, so I'll leave it to you to decide which is better.

I've done it using method ids_independent_equal to not rely on identical order of calling RNG with the exact same seed at the start. There are some things that can be done to prevent randomness like setting random state in game state. After finishing coding I think that saving rng state approach would be probably a bit better but given that the current solution is already done and works I will leave it as it is.
In case you would wonder why there are no tests for ids_independent_equal - broking this methods would cause other tests fail (mostly in AgentEvaluator._check_trajectories_dynamics) so I assumed thay are not as high priority as the new features. Let me know if you think other way - writing few basic unit tests does not seem to be difficult.

"- serving location - 'S'\n",
"- player starting location - number \n",
" \n",
"You can save layout in ovecooked_ai/overcooked_ai_py/data/layouts directory and then run agent evaluator AgentEvaluator({\"layout_name\": layout_name}) where layout_name is filename without \".layoyut extension\". \n",
Copy link
Collaborator Author

@bmielnicki bmielnicki Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This notebook is not up to date with master branch. Probably needs some refinement before merging this file to master - see comment about possible changes in #48 issue

@bmielnicki
Copy link
Collaborator Author

bmielnicki commented Aug 24, 2020

Moving this PR to draft because soon I will change the chart code to add support of adjectives, delivery events, etc.
While I'm changing the visualization code I'm considering refactor to make code cleaner and more maintainable i.e. to make this and similar changes easier - ideally there is no need for modifying js code at all in case of adding a new adjective to events. Mostly it gonna be pushing all of the code distinguishing objects types/event adjectives/players etc into automatically created CSS classes, so d3.js would be used only for generation/modifying of HTML with well-named classes. A nice side effect of it would be allowing users to configure everything that is configurable by CSS.

@bmielnicki bmielnicki marked this pull request as draft August 24, 2020 16:26
@bmielnicki
Copy link
Collaborator Author

I've added new functionalities and did a bit of refactoring.

  1. There is support for giving custom CSS code to create_chart_html function either as a CSS file path or CSS string.

  2. Adjectives are now visible on the chart. My idea for object adjectives was to use borders (as I did not have another idea) of a different colour.

  3. Potting and deliveries have now separate visual representations (ellipse and checkmark).

  4. There is now configurable what cumulative data is visible to the user (you now can choose event adjectives and event actions for each line in cumulative charts).
    It is for parameter "cumulative_data_lines" with value looking like that:
    {"actions":["action1", "action2"], "adjectives":["adj1", "adj2"]}, "name": "Name of line in the legend", "class": "css-class-name"}

  5. A previously big chunk of config that was about CSS properties inside python and js. If it was possible it was moved to the CSS file. If needed I can add a translation of the configuration in python dictionary directly to CSS code in case of most used stuff like chart width.
    The biggest thing I did not move out of js code is the generation of SVG objects with data points (now pickups and drops are always triangles, potting is ellipse and delivery is checkmark and there is no way to check it without modifying js code).

  6. The tutorial notebook is deleted as it would be added when making Tutorial Google Colab Notebook #48 after the merge of native python visualization and this pull request.

Example chart below:
new-event-chart

@bmielnicki bmielnicki marked this pull request as ready for review September 7, 2020 11:00
@bmielnicki bmielnicki dismissed nathan-miller23’s stale review September 25, 2020 11:01

requested changes are already done (tests) or irrelevant (canvas version change)

@bmielnicki
Copy link
Collaborator Author

I've split the event_chart.js file into 2 parts: render_events_chart function (used in both overcooked_demo and this PR) and script to rune render_events_chart from python (template with variables that are sent from python code).
Also, there is an added event that marks the end of the episode (so the x-axis is always the same length as the whole episode perfectly aligning slider handle of the same width).

@micahcarroll
Copy link
Member

Given that this is a lower priority issue right now, I'll temporarily close this PR for bookeeping – we can re-open it in the future if we are interested in exploring this direction further.

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.

3 participants