Skip to content

Conversation

@erh
Copy link
Member

@erh erh commented Nov 8, 2025

…fruit

@erh erh requested a review from dgottlieb November 8, 2025 01:58
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 8, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 8, 2025
return result, nil
}

// composeTransforms assumes there is one moveable frame and its DoF is equal to the `inputs`
Copy link
Member

Choose a reason for hiding this comment

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

Recommend the following comment:

// composeTransforms returns the pose for `frame` given the inputs that frame depends on. The cache
// maps frame names to output poses. The cache assumes the linear inputs are the same as prior calls
// to `composeTransforms`. When a caller changes its `linearInputs`, the caller must invalidate its
// `cache` object.

Copy link
Member

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

Didn't check the callsite updates to check for cache invalidation -- just looked at the composeTransforms bit. Let me know if you think there's value in me taking a closer look for bugs in the usage. I assumed that class of bugs wasn't particularly at risk here.

Update comment for composeTransforms function to clarify its purpose and cache behavior.
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 9, 2025
@erh
Copy link
Member Author

erh commented Nov 9, 2025

Didn't check the callsite updates to check for cache invalidation -- just looked at the composeTransforms bit. Let me know if you think there's value in me taking a closer look for bugs in the usage. I assumed that class of bugs wasn't particularly at risk here.

this had less impact than i expected.
can you take a closer look at some point and see what you think.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 11, 2025
@github-actions
Copy link
Contributor

Availability

Scene # viamrobotics:main erh:20251107-cache-transforms Percent Improvement Health
1 100% 100% 0%
2 100% 100% 0%
3 100% 100% 0%
4 100% 100% 0%
5 100% 100% 0%
6 100% 100% 0%
7 100% 100% 0%
8 100% 100% 0%
9 100% 100% 0%
10 100% 100% 0%

Quality

Scene # viamrobotics:main erh:20251107-cache-transforms Percent Improvement Probability of Improvement Health
1 1.31±0.00 1.31±0.00 -0% 50%
2 0.90±0.00 0.90±0.00 -0% 50%
3 6.63±0.57 6.67±0.61 -1% 48%
4 3.23±0.41 3.23±0.41 -0% 50%
5 8.10±2.32 8.12±2.30 -0% 50%
6 8.63±2.93 8.71±2.80 -1% 49%
7 5.79±2.75 5.79±2.75 -0% 50%
8 0.90±0.00 0.90±0.00 -0% 50%
9 4.15±0.00 4.15±0.00 -0% 50%
10 12.84±0.41 12.84±0.41 -0% 50%

Performance

Scene # viamrobotics:main erh:20251107-cache-transforms Percent Improvement Probability of Improvement Health
1 0.02±0.00 0.02±0.00 -3% 44%
2 0.05±0.00 0.04±0.00 7% 81%
3 0.08±0.02 0.07±0.01 2% 53%
4 1.31±0.11 1.27±0.06 3% 61%
5 1.74±0.43 1.75±0.42 -0% 50%
6 1.96±0.64 1.95±0.73 1% 50%
7 2.38±0.68 2.50±0.75 -5% 45%
8 0.05±0.00 0.04±0.00 5% 78%
9 2.22±0.20 2.20±0.12 1% 54%
10 6.26±1.06 6.00±1.04 4% 57%

The above data was generated by running scenes defined in the motion-testing repository
The SHA1 for viamrobotics:main is: 5385bdb0a6d8e617a53c351316b62a26a2b93ad4
The SHA1 for erh:20251107-cache-transforms is: 5385bdb0a6d8e617a53c351316b62a26a2b93ad4

  • 10 samples were taken for each scene

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test This pull request is marked safe to test from a trusted zone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants