[wpimath] Update units library - #9297
Conversation
github-actions
Bot
added
os: simulation
component: ntcore
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
93ff9f3 to
50bdfe2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50bdfe2603
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -179,7 +177,7 @@ constexpr T CopyDirectionPow(T value, double exponent, | |||
| * the input range. | |||
| */ | |||
| template <typename T, int N> | |||
| requires std::is_arithmetic_v<T> || wpi::units::traits::is_unit_t_v<T> | |||
| requires std::is_arithmetic_v<T> || wpi::units::traits::is_unit_v<T> | |||
There was a problem hiding this comment.
When T is a ratio-scaled dimensionless unit such as percent<>, the unit branch converts vector elements to double as normalized fractions but converts the processed doubles back by invoking T(double), which interprets them as named-unit points. For example, CopyDirectionPow([50_pct], 2, 100_pct) returns 0.25_pct instead of 25_pct; the analogous round trip in the vector ApplyDeadband() overload is wrong by the same scale factor. Convert to and from raw values consistently, or reconstruct results through a base-dimensionless unit before converting to T.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
For now, we're going to disable unit vector with ratio-dimensionless units due to bugs in upstream code.
96759a8 to
8ebfb9b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
d049c78 to
a2283d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2283d4181
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| table.SetProperty(name, "unit", "\"" #abbrev "\""); \ | ||
| table.Log(name, value.value()); \ |
There was a problem hiding this comment.
For ratio-scaled dimensionless units, value() returns the normalized fraction while the emitted metadata names the original unit. For example, logging percent<>{50} writes 0.5 with unit "pct", so telemetry consumers interpret the value as 0.5 percent instead of 50 percent; ppm and ppb have larger errors. Log value.raw() here so the numeric payload matches the advertised abbreviation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We're just going to keep ratio-dimensionless units as normalized fractions.
a2283d4 to
e1944a8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1944a81e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Underlying accumulator = Underlying(0); | ||
| for (Eigen::Index i = 0; i < v.size(); ++i) | ||
| { | ||
| const Underlying raw = v(i).template to<Underlying>(); |
There was a problem hiding this comment.
When the vector scalar is a ratio-scaled dimensionless unit, to<Underlying>() returns the normalized fraction, but line 192 reconstructs Unit as though that fraction were expressed in the named scale. For example, the norm of [3_pct, 4_pct] becomes 0.05_pct instead of 5_pct. Accumulate raw named-unit values, or reconstruct the result through base dimensionless units before converting back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Really? You couldn't have told me this 3 days ago?
|
|
||
| Eigen::Matrix<Underlying, VectorDerived::RowsAtCompileTime, 1> raw; | ||
| for (Eigen::Index i = 0; i < vector.size(); ++i) | ||
| raw(i) = vector(i).template to<Underlying>(); |
There was a problem hiding this comment.
For a ratio-scaled dimensionless vector, this unwraps each element to its normalized fraction and line 275 then interprets the transformed fraction as points in the named unit. Even an identity transform changes [3_pct, 4_pct] into [0.03_pct, 0.04_pct]. Use raw() consistently for the matrix operation, or convert the results from a base dimensionless unit before constructing Unit.
Useful? React with 👍 / 👎.
e1944a8 to
f3ca10e
Compare
f3ca10e to
9a53ac3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a53ac300e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| namespace pybind11 { | ||
| namespace detail { | ||
| template <> struct handle_type_name<wpi::units::radians_per_second_cubed<>> { | ||
| static constexpr auto name = _("wpimath.units.radians_per_second_cubed"); |
There was a problem hiding this comment.
When a binding uses this newly added angular-jerk type, its generated signature or stub refers to wpimath.units.radians_per_second_cubed, but wpimath/units.py defines no such attribute. The same mismatch affects the other newly exported caster families, including jerk, luminance, energy density, irradiance, substance concentration, viscosity, and volume flow rate. Even after updating the renamed legacy aliases, these new annotations remain unresolved for downstream RobotPy bindings; add corresponding float aliases for the newly exported names.
Useful? React with 👍 / 👎.
3 participants
No description provided.