wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.06k stars 614 forks source link

Java Units: Add more cases of dimensional analysis for `times()` and `divide()` #6696

Closed bhall-ctre closed 1 month ago

bhall-ctre commented 4 months ago

This adds/fixes some common cases of dimensional analysis for times() and divide() in the Java units library:

This does not add dimensional analysis when one of the measurements is a Mult<U1, U2>.

WispySparks commented 4 months ago

Measure.divide doesn't check if the other unit is a Time and will return a Per instead of Velocity, a case should be added at the end for this, e.g.

if (other.unit() instanceof Time time) {
      return unit().per(time).ofBaseUnits(baseUnitMagnitude() / other.baseUnitMagnitude());
}
SamCarlberg commented 3 months ago

The changes proposed in this PR, #6710, and #6676 are highlighting the limitations of the generics-based approach that I originally built the library with.

In this PR, we can add more and more cases for dimensional analysis but it's completely impossible to make it type safe due to Java's lack of reified generics. Users need to do type casting on the results, which can lead to errors at runtime if they incorrectly do the dimensional analysis or expect the library to support a particular case that it doesn't.

@narmstro2020's pull requests demonstrate the difficulty that generic types impose on users; specifically that long, complex types are clunky and discourage teams from using the library (which, obviously, we want to avoid!).

I hate to do it, but I feel a rewrite of the library with discrete types for each unit and dimension would resolve these problems. Discrete measurement classes instead of a single Measure type would mean we could add unit-specific math functions to each (eg define Dimensionless.divide(Time) to return a Frequency object, but Distance.divide(Time) to return a LinearVelocity). This would mean, instead of building out a huge set of dimensional analysis cases in a single function that has to return a measure of an unknown unit, users would have access to type safe methods without needing to do type casts or make assumptions - the type system and our classes will handle it for them. Unfortunately it means a lot of new code needs to be written - probably on the order of 5-10 KLOC - and complete breakage from the 2024 API.

narmstro2020 commented 3 months ago

@SamCarlberg It sounds like you would have to move much of the functionality out of the Measure classes into the unit type classes. My PR has probably laid a lot of the groundwork for this in that it adds many more unit types. I avoided frequency because I saw a PR already set up and didn’t want to step on any toes. I would probably get rid of the unit conversion stuff in favor of measurement math generating new measurements, but it can certainly be expanded on and altered in its current form. I’m on vacation this week and can pick this up next week if you’d like to work with me on this or if you think it has any merit.

PeterJohnson commented 1 month ago

Superseded by #6958.