Right shift and division with rounding. #203

stephentyrone · 2021-09-26T20:25:59Z

Fairly correct implementation of right-shift and division with rounding. Also fairly efficient, though I'm sure I'll find some cases where we're generating uncessary overflow checks upon further inspection. Nonetheless, a decent enough start.

The methods are added to BinaryInteger:

shifted<Count: BinaryInteger>(right: Count, rounding: RoundingRule) -> Self
divided(by: Self, rounding: RoundingRule) -> Self

The following methods are added to SignedInteger (not BinaryInteger, because the remainder is not generally representable in an UnsignedType when the rounding mode is not .down or .towardZero):

remainder(dividingBy: Self, rounding: RoundingRule) -> Self
divded(by: Self, rounding: RoundingRule) -> (quotient: Self, remainder: Self)

The following free function is added:

euclideanDivision<T: BinaryInteger>(_: T, _: T) -> (quotient: Self, remainder: Self)

Fairly correct implementation of right-shift with rounding. Also fairly efficient, though I'm sure I'll find some cases where we're generating uncessary overflow checks upon further inspection. Nonetheless, a decent enough start.

stephentyrone · 2021-09-26T20:26:58Z

@swift-ci test

stephentyrone · 2021-09-27T01:48:14Z

@swift-ci test

stephentyrone · 2021-09-27T01:55:06Z

@swift-ci test

Catfish-Man

Reviewed API only, didn't look at the implementation at all

Catfish-Man · 2021-09-27T17:46:50Z

Sources/IntegerUtilities/Rounding.swift

+
+/// A rule that defines how to select one of the two representable results
+/// closest to a given value.
+public enum RoundingRule {


This feels a little inconsistent.

…, rounding: .down ✅
..., rounding: .stochastic should this be "stochastically"?
..., rounding: .trap in what circumstances does it trap? (I realize this is clear if you think about it for a moment, but maybe we can make it obvious at the point of use?)

Related, do we want rounding or do we want rounded? With the name shifted we're describing the resulting value, rather than the action, but with rounding we're describing the action.

Yeah, I'm not super-delighted with "trap" either. Still trying to come up with a better name for it. In the meantime, it has the virtue of being short, and strongly suggesting that it can trap, so that's not a surprise.

trapIfUnrepresentable?

.exactOrTrap maybe?

Maybe just .exact, though I would like to mention "trap" in the name. I like the symmetry of exact with the exactly: inits from the stdlib.

I like .exactOrTrap or something similar. "Trap" is kind of a specialist word, though, so maybe .exactOrFatal? .exactOrAssert? .mustBeExact? I think it makes sense to have this one be linguistically distinct, so .onlyExact or .mustBeExact or .remainderMustBeZero or ...

If this was implemented in the standard library, would the new cases be added to FloatingPointRoundingRule, or should the new enum be renamed to IntegerRoundingRule instead?

I would like to deprecate FloatingPointRoundingRule eventually and replace it with RoundingRule. Exactly what that migration path looks like isn't totally clear, but I think it's the direction that we'll take if/when these show up in the stdlib.

The existing FloatingPointRoundingRule has a few problems:

it's an open enum, which has some not-ideal representation/performance tradeoffs for Swift. It should have been a frozen struct with static vars instead (so the layout could be frozen while still being able to add values), but that pattern wasn't yet widely used when the type was added.

rounding rules generally make sense for integers as well as floating-point (see this API). Even if a given rule is more/less efficient to implement or more/less useful for one type or another, it's nice to have a uniform set.

Catfish-Man · 2021-09-27T17:47:40Z

Sources/IntegerUtilities/Shift.swift

+  ///     5.shifted(right: 1, rounding: .trap)
+  @inlinable
+  public func shifted<Count: BinaryInteger>(
+    right count: Count,


I forget, what's the precedent here: do we usually include or omit "By"?

Catfish-Man · 2021-09-27T17:49:20Z

Sources/IntegerUtilities/Shift.swift

+  @inlinable
+  public func shifted<Count: BinaryInteger>(
+    right count: Count,
+    rounding rule: RoundingRule = .down


As someone who doesn't think about rounding very often I'm just going to assume that you've thought about this and there's an obvious default here, but I'm leaving this comment just on the tiny chance that on seeing it again you'll go "hm actually maybe they should always have to say what they want" :)

For right shift, the default should certainly be to truncate. That's down or towardsZero for positive values. I have to stop and think for a bit before I can remember the right behavior for negative values.

Right; "normal" right shifts (>>) round down, and there's no good reason to make anything else the default. "Normal" division (/) rounds toward zero, which is why divide by power of two can't be replaced with a shift for signed values, but the way to resolve that mismatch is to make the forthcoming divide-with-rounding default to .down as well, as that gives a more sensible remainder for most uses.

milseman · 2021-09-27T17:50:20Z

Sources/IntegerUtilities/Rounding.swift

+  ///
+  /// This is the default rounding mode for integer shifts, including the
+  /// shift operators defined in the standard library.
+  case down


What do you think about additionally providing something like:

static var `default`: Self { .down }

I think the main concern would be that we might add an operation someday that wants to use a different default. round-to-nearest multiple comes to mind as a possibility.

Ah, so default depends on operation (so either current design or name it defaultShifting, etc.). What about a "fastest" or "I don't care" computed var, would that also be dependent on the operation?

"don't care" here would just be >>, and for divide it would be /. Expectation is that people reach for these when they do care.

glessard · 2021-09-27T18:20:28Z

Sources/IntegerUtilities/Shift.swift

+      // have to take the RNG in-out. The same problem applies to rounding
+      // with dithering. We should consider adding a stateful rounding API


Is mentioning dithering relevant? I find it confusing, especially since I thought that it is exactly this case.

The // comments are mostly notes for myself, rather than public documentation. =) Referring to error-diffusion or ordered dithering here, though that isn't obvious.

I'm not sure I'd include stochastic rounding in this API. It raises too many questions. (Ulichney's "Digital Halftoning" is a great introduction that discusses the importance of controlling the frequency distribution of your noise source.)

Blue noise and the like and ordered dithers require a stateful rounding API, but even stochastic rounding without state is quite useful for some purposes.

Also implemented some basic statistical tests for .stochastic and reorganized code a little bit. The statistical justification of the thresholds for these tests should be explained a bit more, and we should decide on a policy in general for statistical tests that may fail sporadically, but these shouldn't be very noisy for now.

stephentyrone · 2021-09-28T15:30:52Z

@swift-ci test

stephentyrone · 2021-10-12T21:17:12Z

@swift-ci test

Also simplifies check logic for divide with rounding, which is a modest speedup for testing.

stephentyrone · 2021-10-14T19:00:25Z

@swift-ci test

stephentyrone · 2021-10-14T19:19:26Z

@swift-ci test linux

Skeleton of integer shift with rounding.

c124585

Fairly correct implementation of right-shift with rounding. Also fairly efficient, though I'm sure I'll find some cases where we're generating uncessary overflow checks upon further inspection. Nonetheless, a decent enough start.

stephentyrone marked this pull request as draft September 26, 2021 20:26

stephentyrone added 3 commits September 26, 2021 17:05

Some documentation work.

d78c067

More rounding mode documentation.

eeaf9e3

Cursory documentation for shifted(right:).

92eb762

stephentyrone marked this pull request as ready for review September 27, 2021 01:47

stephentyrone changed the title ~~Skeleton of integer shift with rounding.~~ Right shift with rounding. Sep 27, 2021

Re-arrange some things.

cddd5e6

stephentyrone force-pushed the rounding branch from 11add40 to cddd5e6 Compare September 27, 2021 01:54

Catfish-Man reviewed Sep 27, 2021

View reviewed changes

milseman reviewed Sep 27, 2021

View reviewed changes

glessard reviewed Sep 27, 2021

View reviewed changes

stephentyrone changed the title ~~Right shift with rounding.~~ Right shift and division with rounding. Oct 12, 2021

Added division with rounding operations.

501f582

stephentyrone force-pushed the rounding branch from 2a9d2a1 to 501f582 Compare October 12, 2021 21:17

stephentyrone added 3 commits October 13, 2021 11:08

Renamed shifted(right:) to shifted(rightBy:).

710ca08

Additional documentation for division/shift with rounding

3eaa0b3

Also simplifies check logic for divide with rounding, which is a modest speedup for testing.

Add tests for remainder(dividingBy: -1).

24d8f51

stephentyrone merged commit f956c23 into apple:main Oct 15, 2021

stephentyrone deleted the rounding branch October 15, 2021 15:40

stephentyrone mentioned this pull request Oct 15, 2021

Integer utilities module #10

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Right shift and division with rounding. #203

Right shift and division with rounding. #203

stephentyrone commented Sep 26, 2021 •

edited

Loading

stephentyrone commented Sep 26, 2021

stephentyrone commented Sep 27, 2021

stephentyrone commented Sep 27, 2021

Catfish-Man left a comment

Catfish-Man Sep 27, 2021

stephentyrone Sep 27, 2021 •

edited

Loading

rxwei Sep 27, 2021

xwu Sep 27, 2021

stephentyrone Sep 27, 2021

tbkka Sep 27, 2021

benrimmington Sep 28, 2021

stephentyrone Sep 28, 2021 •

edited

Loading

Catfish-Man Sep 27, 2021

Catfish-Man Sep 27, 2021

tbkka Sep 27, 2021

stephentyrone Sep 27, 2021

milseman Sep 27, 2021

stephentyrone Sep 27, 2021

milseman Sep 27, 2021

stephentyrone Sep 28, 2021 •

edited

Loading

glessard Sep 27, 2021

stephentyrone Sep 27, 2021 •

edited

Loading

tbkka Sep 27, 2021

stephentyrone Sep 27, 2021 •

edited

Loading

stephentyrone commented Sep 28, 2021

stephentyrone commented Oct 12, 2021

stephentyrone commented Oct 14, 2021

stephentyrone commented Oct 14, 2021

		// have to take the RNG in-out. The same problem applies to rounding
		// with dithering. We should consider adding a stateful rounding API

Right shift and division with rounding. #203

Right shift and division with rounding. #203

Conversation

stephentyrone commented Sep 26, 2021 • edited Loading

stephentyrone commented Sep 26, 2021

stephentyrone commented Sep 27, 2021

stephentyrone commented Sep 27, 2021

Catfish-Man left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

stephentyrone commented Sep 28, 2021

stephentyrone commented Oct 12, 2021

stephentyrone commented Oct 14, 2021

stephentyrone commented Oct 14, 2021

stephentyrone commented Sep 26, 2021 •

edited

Loading

stephentyrone Sep 27, 2021 •

edited

Loading

stephentyrone Sep 28, 2021 •

edited

Loading

stephentyrone Sep 28, 2021 •

edited

Loading

stephentyrone Sep 27, 2021 •

edited

Loading

stephentyrone Sep 27, 2021 •

edited

Loading