Conversation
Use agent, action and reward information for an episode to create some initial, simple plots.
Points of interest are currently just any step that received a reward for one (or more) agent(s). Clicking on a point will jump to that point of interest, displaying the current frame and information for any associated agent steps (agent name, action, and reward).
This will very likely need to be re-thought as the project grows, but for now we can simply hard-code mappings of actions to string representations for environments with discrete action spaces.
- Stagger text to handle crowded timelines - Format float values
bnmajor
force-pushed
the
new-ui-features
branch
from
77df6e0 to
f55ff2c
Compare
- Use dot operator rather than subscripts - Use better naming - Leverage Python's standard library to simplify code
bnmajor
force-pushed
the
new-ui-features
branch
from
f55ff2c to
69bbb66
Compare
| plot_data['action_v_reward'][entry.agent][action] += entry.total_rewards | ||
| plot_data['action_v_frequency'][action] += entry.action_frequency | ||
|
|
||
| key_steps = steps.annotate(total_rewards=Sum('agent_steps__reward', default=0)).order_by( |
There was a problem hiding this comment.
Could this be combined with the original steps definition, so we don't need to execute 2 separate queries when steps and key_steps are evaluated? The extra .annotate shouldn't impact the use of steps, but maybe you don't want steps to have a different ordering by number (although that seems like a sensible ordering)?
There was a problem hiding this comment.
However, if you think it's better to keep these logically separated to make future refactoring easier, that's a reasonable price to pay for one more query.
There was a problem hiding this comment.
As you mentioned, these have remained separate queries for the ease of refactoring. This "any reward was received" as a metric for determining points of interest should be dropped altogether in the future in favor of more advanced insights once they've been added.
| <div class="content-center w-1/2 h-full min-h-full"> | ||
| <img class="max-w-full h-auto" src="{{ step.image.url }}" alt="Step Image"> | ||
| <div class="grid grid-cols-12"> | ||
| <div class="col-span-3 content-center" x-data="{ |
There was a problem hiding this comment.
Would it help to split each of these plots into its own template and {% include them? I'm not sure if that would be more readable, but I think this file is getting pretty big.
There was a problem hiding this comment.
Yes, I think that would make sense! I did not do that in this PR, but my WIP to produce aggregated results across many episodes does include changes to break out components and then include them as needed.
| :disabled="currentStep === 0" | ||
| @click="currentStep = 0" | ||
| > | ||
| <!-- Rewind Icon --> |
There was a problem hiding this comment.
It might be a lot easier to use an icon library. I've used https://remixicon.com/ for several other server-rendered applications.
You should just need to add
<link href="https://unpkg.com/remixicon/fonts/remixicon.css" rel="stylesheet"/>then you'll be able to use something like
< i class="ri-rewind-line"></i>instead of all this SVG.
There was a problem hiding this comment.
I have a follow-up branch with additional cleanup for styles. I will include this change in that PR.
| </div> | ||
| <div class="grid grid-cols-12"> | ||
| <div class="col-span-6"> | ||
| {% for step in steps %} |
There was a problem hiding this comment.
Assuming that the step-based views are going to continue, I think we should definitely switch to a more scalable method for rendering this much data. Emitting this much HTML at once may be problematic for many users (I'm worried we're already in "it works on my machine" territory, given that developers have over-powered machines relative to many users).
#19 is one approach, though I'm happy to discuss others.
This change is out of scope for this PR, but it'd be good to get this fixed ASAP, unless we're not planning to allow the selection of individual steps (or things of equivalent cardinality) anymore.
There was a problem hiding this comment.
I've opened this up as a new issue for follow-up once this PR is in.
| key_steps = steps.annotate(total_rewards=Sum('agent_steps__reward', default=0)).order_by( | ||
| 'number' | ||
| ) | ||
| plot_data['rewards_over_time'] = list(accumulate(ks.total_rewards for ks in key_steps)) |
There was a problem hiding this comment.
I think you can make the database do this for you with a Window function, something like
Window(Sum('total_rewards'), order_by='number')There was a problem hiding this comment.
I was only able to get this approach to work with a sub-query (apparently you cannot pass an aggregate into a Window function) and this consistently increased both my query times as well as time to load the page so I did not make this change in this PR. Open to suggestions though if I'm maybe just missing something here...
- Use more meaningful variable names - Simplify code to improve readability - Use unpkg for all packages
bnmajor
force-pushed
the
new-ui-features
branch
from
76d26ae to
4f79229
Compare
bnmajor
force-pushed
the
new-ui-features
branch
from
4f79229 to
9490131
Compare
|
@brianhelba I'm going to merge this in for now so that but please let me know if you have any additional concerns and I can address them in some follow-up PRs. |
2 participants
Fixes #22, #23, #26