-
Notifications
You must be signed in to change notification settings - Fork 22
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
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.
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.
Just one note from me re: the intro on Applying Magnitudes — I don't necessarily think it's a blocking issue, but it's something we should revisit when convenient.
@@ -0,0 +1,136 @@ | |||
# Applying Magnitudes | |||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch --- WDYT about 21db19d?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
T
of 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
2
andpi
. 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.