[Lang] Make floor, ceil and round accept a dtype optional argument

[neozhaoliang]

Currently the integer returned by floor function has float type, this PR adds an option to the returned value type.

[netlify]

✅ Deploy Preview for docsite-preview ready!

Name	Link
🔨 Latest commit	529427f
🔍 Latest deploy log	https://app.netlify.com/sites/docsite-preview/deploys/62c8e96d85bff60009a89db0
😎 Deploy Preview	https://deploy-preview-5307--docsite-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[ailzhang]

OOC: what's the motivation for this change?

[neozhaoliang]

OOC: what's the motivation for this change?

A common need in simulations and renderings is that, for a point p we want to find the cell index i, j that p falls into, and use that index to access field elements. Hence allowing return integer types would be more convenient here.

[ailzhang]

LGTM! Would you mind add an example with dtype specified in the docstring, as well as some test cases like test_random_int?

[strongoier]

I doubt whether this change is necessary. Does ti.floor(x, int) have a clear advantage over int(ti.floor(x))?

[sjwsl]

I doubt whether this change is necessary. Does ti.floor(x, int) have a clear advantage over int(ti.floor(x))?

Agree. What's worse is that the range of dtype may be not big enough, e.g. f32->i32, which is UB. It should be users' responsibility to use the result correctly and avoid overflow. floor in C++ (and all the statically typed languages I know) always returns float or double.

[yuanming-hu]

I'd actually vote for a function like ti.floor(val, dtype) given its simplicity, but I'm open to discussions.

I doubt whether this change is necessary. Does ti.floor(x, int) have a clear advantage over int(ti.floor(x))?

Sometimes using int to hold the result of ti.floor may not be what the user wants and you may want to explicitly specify ti.i32/ ti.u64 etc as the result type.
I guess if implemented correctly, ti.floor(x, ti.i64) would be clearer than ti.cast(ti.floor(x, ti.i64)).
The other option is that we allow users to directly cast values/vectors using a syntax like ti.i32(x).

What's worse is that the range of dtype may be not big enough, e.g. f32->i32, which is UB.

IMHO this is actually not making things worse :-) You need such a cast anyways in many use cases.

[k-ye]

What's worse is that the range of dtype may be not big enough, e.g. f32->i32, which is UB.

I think the point is that f32 has a much larger range than INT_MAX/2,147,483,647? That opens up the door for UB in the API...

[sjwsl]

I think the point is that f32 has a much larger range than INT_MAX/2,147,483,647? That opens up the door for UB in the API...

Yeah, but users can do some custom checks to be safe. For example:

template <std::integral T>
std::optional<T> ifloor(double num) {
    auto double_result = std::floor(num);
    auto max = std::static_cast<double>(<std::numeric_limits<T>::max); // may have precision issues, just for demonstration.
    auto min = std::static_cast<double>(<std::numeric_limits<T>::min);
    return double_result <= max && double_resuld >= min ? std::static_cast<T>(double_result) : std::nullopt;
}

Of course we can do something similar inside the API to avoid UB, but I don't think there is a generally accepted logic here. IMHO that's why std::floor in C++ just returns what it accepts.

IMHO this is actually not making things worse :-) You need such a cast anyways in many use cases.

I do agree it won't overflow and we just cast it immediately in most cases. My point is that there should be as few APIs as possible with potentially undefined behavior, especially in the standard library. It's probably ok if thinking of it as a syntactic sugar and it does save some typing in many cases. But if UB is expected, at least it shall be documented.

[KuribohG]

I would like to say the syntax ti.floor(x, int) is more self-documented than ti.i32(ti.floor(x)). A newbie user will have to refer to the document for casting in Taichi if we adopt the latter syntax, while only need to refer to the signature for the former one, which can be done in IDE or IPython.

UB isn't a matter here if we guarantee the float version of this function is well-defined. Like many C++ standard functions, unsafe versions are provided, like std::span didn't have a boundary check, and std::thread didn't provide a default destructor.

[yuanming-hu]

What's worse is that the range of dtype may be not big enough, e.g. f32->i32, which is UB.

I think the point is that f32 has a much larger range than INT_MAX/2,147,483,647? That opens up the door for UB in the API...

That's true, but I believe the user will always get a chance to use ti.floor(x), without specifying the dtype argument, so that the type of x is returned. The current implementation has an issue, since if float = ti.f32 and x is ti.f64, he gets an f32 return value. This should be fixed.

[strongoier]

I'd actually vote for a function like ti.floor(val, dtype) given its simplicity, but I'm open to discussions.

This change can indeed make user code slightly shorter, but IMHO a function should have single responsibility. If we decide to add a dtype parameter to ti.floor, are we going to repeat this (creating an optional casting step) for all functions?

The other option is that we allow users to directly cast values/vectors using a syntax like ti.i32(x).

The syntax ti.f32(1.3) is used to annotate the type of a literal (like 1.3f in C), which is NOT a type cast. IMHO mixing two different semantics in one syntax is confusing.

The current implementation has an issue, since if float = ti.f32 and x is ti.f64, he gets an f32 return value. This should be fixed.

Could you provide an example running into the bug? IIRC the current implementation returns a value of the same type as the input value.

[yuanming-hu]

If we decide to add a dtype parameter to ti.floor, are we going to repeat this (creating an optional casting step) for all functions?

Probably we only do this for floor, since this is the only floating-point function that takes a float but users typically want an int output.

[strongoier]

If we decide to add a dtype parameter to ti.floor, are we going to repeat this (creating an optional casting step) for all functions?

Probably we only do this for floor, since this is the only floating-point function that takes a float but users typically want an int output.

Maybe ceil and round as well?

[k-ye]

If we decide to add a dtype parameter to ti.floor, are we going to repeat this (creating an optional casting step) for all functions?

I think this is a valid point. If floor has a dtype, at least I would expect that ceil has the same interface for symmetry.

That's true, but I believe the user will always get a chance to use ti.floor(x), without specifying the dtype argument, so that the type of x is returned.

I agree.

---

Now giving it more thoughts, I think one possibility that C floor doesn't have an type cast is probably because it can't (without generics or type-as-a-parameter mechanism), i.e. there's no way for it to get a dtype info. If we look at numpy.floor (or any of its ufunc), there is a casting policy:

• https://numpy.org/doc/stable/reference/generated/numpy.floor.html#numpy-floor
• https://numpy.org/doc/stable/user/basics.ufuncs.html#type-casting-rules.

So my current opinion is that we don't have to abide to C's API design, but we should do the same for floor and ceil.

[neozhaoliang]

Added dtype option to ceil and round functions.

[yuanming-hu]

Thank you! The implementation LGTM, but could you update the doc and add a few test cases?

[neozhaoliang]

test script added.

[yuanming-hu]

LGTM in general. Please also update https://docs.taichi.graphics/docs/operator#other-arithmetic-functions.