Refine and document conversion factor policies #176
Conversation
Before landing, should add tests and either put ApplyMagnitude in detail namespace, or document it. This first commit is just to measure compile time impact.
We can handle magnitudes which are equivalent to either multiplying or dividing by an exact integer.
This only makes sense for non-integers, so we check that.
chiphogg
added
release notes: 🐛 lib (bugfix)
| @@ -0,0 +1,136 @@ | |||
| # Applying Magnitudes | |||
|
|
|||
There was a problem hiding this comment.
This page is all about Magnitudes, but the word "magnitude" itself isn't explicitly stated or mentioned in the introduction -- we don't see the word again until the Magnitude Categories section.
Is it possible that this introduction should include the content in the Magnitude Categories section?
There was a problem hiding this comment.
Nice catch --- WDYT about 21db19d?
There was a problem hiding this comment.
That likely contextualizes the rest of the page well. Thank you!
|
Adding @geoffviola for reviewing the code changes. |
There was a problem hiding this comment.
Makese sense. Tests look good.
There are tests with pi. Not sure if we need explicit performance tests around revolutions and radians. It might be nice to characterize precision and performance of the various types. Since we don't expect these conversions to be in a hot loop, it might not be as important.
I'm happy with just double checking godbolt after it lands. 🙂 |
Looks like we're good: https://godbolt.org/z/jjdf7v8dq |
So far, we've applied our conversion factors by first breaking them into
three parts:
num, the integer part of the numeratorden, the integer part of the denominatorirr, the "leftover" irrational parts, if anyWe've applied it as
value * num / den * irr.This breakdown isn't really uniquely defined if
irr != 1, of course.But the idea was that this was "good enough to get started", and we
could count on the compiler to optimize things away in individual cases.
We now replace this with a more thoughtful, principled approach. It
takes into account both the numeric category of conversion factor
(integer, irrational, etc.), and the type
Tof the variable we'rescaling.
This was motivated by looking at godbolt while making the slides for
CppCon 2023, and noticing that sometimes our conversion factors used two
instructions, where the equivalent "no units library" code used only
one.
The specific instance was a "crystal clear" poor case converting between
revolutions and radians, where we applied the irrational "two pi" as a
separate
2andpi. I also investigated the "harder" case of arational factor applied to floating point, and decided that a single
instruction made sense here too.
I also included a detailed discussion doc explaining these rules and
tradeoffs. It's an "implementation" level doc, so it gets pretty into
the weeds, but I think certain users will find it really interesting.
Fixes #173.