-
Notifications
You must be signed in to change notification settings - Fork 3
feat: interactive plot for pointcloud hypothesis space #15
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
base: main
Are you sure you want to change the base?
feat: interactive plot for pointcloud hypothesis space #15
Conversation
fix: pretrained models class descriptions and shim unpickler fix: remove print message in widgets.py feat: extract_slider_state round parameter feat: add extract button state helper function feat: add interactive pointcloud plot
|
Love the visualization! Just some high level comments before I have a chance to review the code. These are generally focused on how we can make it more user friendly or the improve the aesthetics:
Stylistically, I wonder if you could try some subtle changes to improve it a bit? E.g.
|
|
Thanks @nielsleadholm! I addressed the majority of these concerns.
Yes, that makes a lot of sense, especially because the patch can also be called "sensor patch" sometimes, which can be confused with the sensor. Updated in d5427f1c032a
Unfortunately, I only have a few very basic widgets in vedo and even customization can be difficult, drop down menus are not supported. I updated to the pattern
Updated in 9a9132555099
I'm trying to avoid system fonts and only use ones that come builtin in vedo to make sure it works out of the box for everyone. I personally think
This is one of the customizations that's not supported in Vedo.
Nice idea, I added the TBP color palette as part of the interactive library in 784d6fb0bffd and used it in the plot in b9aa9135fa09. I tried to use it wherever possible, hopefully the colors are not too distracting. What do you think?
I fixed this in 8495f5d04f10
I tried this and didn't get any good suggestions that can be done in Vedo. I think the main problem is that Vedo is limited in the types of widgets and possible customizations, we are already using all the options that can be used and pushing vedo to it's limits. Check out their gallery, it's very basic. |
|
Nice thanks, that's looking great! Re. the font, is it possible to try Comae and Normografo for comparison? Otherwise all the changes / responses look good. |
nielsleadholm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice and clean, looks great! Just a few minor comments.
| label="Current step", | ||
| ) | ||
|
|
||
| ax_left.set_ylabel("Max slope") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also be worth changing to "Max Recent Evidence Growth".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks. Fixed in 54517032c4fb
| """Load a torch checkpoint with optional fallback for tbp.monty shimming. | ||
| Try a standard torch.load first (weights_only=False because we need objects). | ||
| If tbp.monty isn't installed and the checkpoint references it, optionally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason tbp.monty wouldn't be installed if using these plotting tools? Was this something you came across?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, tbp.plot uses a different uv environment that does not contain tbp.monty in it. One reason is the mismatch in python versions, tbp.plot uses python 3.13 (not 3.8). But also, ideally, we wouldn't want to depend on the tbp.monty codebase since everything we need is the log files and pretrained models.
In the future, I imagine it would be really cool to have visualizations that actually run a Monty episode step by step from Vedo. This would allow us to change parameters, swap/manipulate objects, move sensors, etc., between steps. At this point, we would have to install tbp.monty.
| class StepMapper: | ||
| """Bidirectional mapping between global step indices and (episode, local_step). | ||
| Global steps are defined as the concatenation of all local episode steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, did you manage to identify the cause of the issue where, after a jump-to-goal-state, the agent seemingly moves to a new location, while the sensory location remains un-updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, it seems like a problem on the logging side of tbp.monty. I'll look into it this week, shortcut ticket created.
| } | ||
|
|
||
| self.step_mapper = step_mapper | ||
| self.current_episode = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a significance to setting it to -1? Could it just be None? Seeing this value makes me nervous that there is some unexpected logic dependent on it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, I get a little nervous when comparing None to int because I think python will error out and say that it cannot compare these two different types. I'm wrong, python is fine with it. I still do prefer to limit the number of types a variable can be (int vs int | None), but I guess that's just nitpicking on my side. I changed it to None in de62bdd9bea6.
| patch_pos = self.data_parser.extract( | ||
| self._locators["patch_location"], | ||
| episode=str(episode_number), | ||
| step=step_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that the agent-pose uses int(mapping[step_number]), while here we just use step_number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the agent_location locator is loaded from the motor_system logs, those are not filtered by which ones are processed by the LMs. So, I get the mask of which steps are processed first and use this mapping to extract the correct agent locations for the steps processed by the LM. The sensor locations come from the buffer, so we don't need this mapping since it only logs the ones processed by the LM.
| ax_left.set_ylim(-1.0, 2.0) | ||
| ax_right.set_ylim(0, 10000) | ||
|
|
||
| # Burst locations (red dashed lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Burst locations (red dashed lines) | |
| # Burst locations (violet dashed lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 54517032c4fb
| """WidgetOps implementation for the line plot. | ||
| This widget shows a line plot for the max global slope and hypothesis space size | ||
| over time. It also shows the sampling bursts locations as vertical dashed red lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| over time. It also shows the sampling bursts locations as vertical dashed red lines. | |
| over time. It also shows the sampling bursts locations as vertical dashed violet lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 54517032c4fb
| - Buttons to activate or deactivate the history of agent and/or patch locations. | ||
| - A plot of the pretrained model for the primary target with a button to show the | ||
| hypothesis space for this object. | ||
| - The hypothesis space pointcloud can be coloured by different metrics, such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - The hypothesis space pointcloud can be coloured by different metrics, such as | |
| - The color-map of the hypothesis space pointcloud can reflect different metric values, such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 54517032c4fb
|
Great, thanks @nielsleadholm! I think I addressed your comments. Can you take another look? |










This introduces a new hypothesis space visualization with a focus on burst sampling. The line plot shows when a burst is triggered, as well as the hypothesis space size and the max global slope over time. I've also introduced toggle switches to view the history of agent and sensor patch locations. There are some other nice features such as alpha slider for the mesh, and coloring the hyp space point cloud by different metrics. The video below showcases some of those features.
FYI @hlee9212, I've added your Shim Unpickler solution to the interactive library as part of a
PretrainedModelsLoaderclass that returns a point cloud of the pretrained model. Feel free to adjust your code to use that directly if you like.line_plot.mp4