Skip to content
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

Add discussion of overflow and how to mitigate it #203

Merged
merged 7 commits into from
Dec 20, 2023
Merged

Conversation

chiphogg
Copy link
Contributor

This finally --- finally! --- gives us an authoritative place to link for
explaining the "overflow safety surface". I was going to make that the only
topic of the page, but as I wrote I realized that there's a lot more value in
discussing the overflow problem more generally! I expect this link will be a
useful reference for other units libraries as well.

I adapted some contents that were hidden away a couple layers deep in the 103
tutorial, and replaced those contents with a link to the new page.

This finally --- _finally!_ --- gives us an authoritative place to link
for explaining the "overflow safety surface".  I was going to make that
the topic of the page, but I realized that there's a lot more value in
discussing the overflow problem more generally!  I expect this link will
be a useful reference for other units libraries as well.

I adapted some contents that were hidden away a couple layers deep in
the 103 tutorial, and replaced those contents with a link to the new
page.

This helps #90 tangentially, in that I wanted to link to this page in
the docs for `Constant`, but was stymied by its nonexistence.
@chiphogg chiphogg added the release notes: 📝 documentation PR affecting library documentation label Nov 28, 2023
@chiphogg chiphogg requested a review from tobin November 28, 2023 01:37

Fundamentally, there are two contributions to the level of overflow risk:

1. The _size of the conversion factor_: **bigger factors** mean **more risk**.
Copy link

Choose a reason for hiding this comment

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

In the example you gave earlier, it's not the magnitude of the conversion factor between meters and yards that is the problem (indeed that conversion factor doesn't even exist in the integral domain) but the conversion factor to the implied common unit, or the magnitudes of the numerator and denominator of the conversion factor. Not sure whether this needs to be called out explicitly, but I think you have segued somewhat from the original motivating example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I ended up going for a footnote here. I considered discussing the kind of overflow that can happen with rational conversion factors applied to integral-rep quantities. However, in the context of this section, we're talking about conversions with safety checks applied... and rational conversion factors would already fail the safety check for truncation, so they're not really in scope here.

How small is "scary"? Here are some considerations.

- Once our values get over 1,000, we can consider switching to a larger SI-prefixed version of the
unit. (For example, lengths over $1000\,\text{m}$ can be approximated in $\text{km}$.) This
Copy link

Choose a reason for hiding this comment

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

This mention of "approximation" sort of triggers my anxiety. "Approximation?!! I thought our unit library was exact!" etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I switched it from "approximated" to "more concisely expressed". I hope it makes for a smoother read. 🙂

One way to _guarantee_ doing better is to check every conversion at runtime. Some users may recoil
at the idea of doing _runtime_ work in a units library, but it's easy to show that _this_ use case
is innocuous. Consider: it's very hard to imagine a valid use case for performing unit conversions
in a "hot loop". Therefore, the extra runtime cost --- merely a few cycles at most --- won't
Copy link

Choose a reason for hiding this comment

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

Care must be taken to avoid "implicit" unit conversions inside a hot loop, no? For example your motivating example of evaluating a>b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Tweaked "performing" to "needing to perform". I didn't see an easy way to bring up the implicit conversions explicitly here without hurting the flow.

(But yeah, unintended implicit conversions could hurt performance. I think the approach here would be something like what I used to hear at Google: "if you care about performance, and you don't have a benchmark, then you don't care about performance". 😁 I also got a question at CppCon 2023 about the possibility of making all unit conversions explicit --- which, although we're not planning to do it, would solve this kind of problem.)

Copy link

Choose a reason for hiding this comment

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

Is it obvious that the points on the line are "permitted"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It certainly wasn't, so I added a legend.

I also tweaked the text to use the same phrase as the legend. (I guess I could have just used the original text in the legend, but I didn't see it until I was done updating the figure.)

Copy link

@tobin tobin left a comment

Choose a reason for hiding this comment

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

As always, a pleasure to read, and I always learn something new! (I always assumed that feet and meters must have some irrational or at least many-significant-figures conversion factor, for example. Thus it was a nice surprise to see how the conversion can be done with integral types.)

I also included a couple of wording tweaks based on text that caught my
eye as I re-skimmed.
@chiphogg chiphogg merged commit ea08978 into main Dec 20, 2023
9 checks passed
@chiphogg chiphogg deleted the chiphogg/overflow branch December 20, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: 📝 documentation PR affecting library documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants