ceil and floor are dangerous

[BenFrantzDale]

What's the ceiling of my height? Much like log, ceil and floor don't make sense for strongly-united types. I could see ceil_in<unit>, but just ceil and floor aren't invariant against changes in the unit used.

I think these are the similar ones:

• ceil
• floor
• round
• trunc
  I guess they are all transcendental?

[mpusz]

ceil() was added to be consistent with std::chrono::duration. However, I agree that this is similar to our discussion about .number() and .number_in(Unit) here: #412.

[mpusz]

For the sake of generic programming, we probably should not remove ceil() and floor() as a user might now write a generic code like:

auto foo(auto v) { return ceil(v); }

and the above will work with arithmetic types, chrono::durations, units::quantity, and possibly other types as well.

[mpusz]

We can consider adding unit-aware alternatives as we did for .number_in(Unit). The only question I see here is how the interface should look like.

Option A

ceil_in(metre, q);

This is consistent with .number_in(Unit), which takes the unit as a function parameter (to prevent issues with the disambiguation of dependent names). We can also reorder the arguments here.

Option B

ceil_in<metre>(q);

This is probably more readable but inconsistent with .number_in(Unit).

[chiphogg]

I like Option A, partially because it's what we use in Au. 🙂 Here are some representative test cases to show example usages:

https://github.com/aurora-opensource/au/blob/703629f493a4a976cce7ce4c6e720f6617425093/au/math_test.cc#L660-L690

The main difference is that we spell it ceil_as(units, q), not ceil_in(units, q). That's because of our idiom that "in" is a word that retrieves raw numbers, and "as" is a word that stays within the "quantity domain". (We also have ceil_in(units, q) which gives a raw number.)

I also think it's valuable to be consistent with number_in(units).

[chiphogg]

For the sake of generic programming, we probably should not remove ceil() and floor() as a user might now write a generic code like:

auto foo(auto v) { return ceil(v); }

and the above will work with arithmetic types, chrono::durations, units::quantity, and possibly other types as well.

I don't really buy that argument. I think the fact that it works with chrono::duration is a bug.

It's meaningful to ceil a number, but not to ceil a (notional) "quantity". I think it's good to prevent this kind of generic code from working. (Incidentally, I'm hard pressed to think of a use case for that code where we'd be glad if it worked for quantities, or durations for that matter.)

[chiphogg]

Also, to comment directly on the original issue post: yes, exactly! And we have real world implementation experience that validates this approach (i.e., to provide round_in(units, q) instead of round(q), etc.) We have unit-aware versions of round, ceil, and floor (not for trunc, but that's just an oversight). Making units explicitly part of the API has been a significant improvement in safety and readability, and hasn't really caused any issues.

[mpusz]

@chiphogg, thanks for the valuable feedback!

I don't really buy that argument. I think the fact that it works with chrono::duration is a bug.

Well, I was waiting for someone to say that 😉 It was just some rough idea of why it might be useful, but I also do not see a use case for that.

I will change that in V2 soon.

[BenFrantzDale]

The one similar cmath function I would provide is nextafter. Like ceil and friends, it’s not invariant under change of units, but conceptually it finds a value that is bigger (or smaller) in a way that’s conceptually x + epsilon. It lets you turn a closed–closed bounding box into a half-open one.

[mpusz]

Done in V2.