warn about using floor division of durations

[dirk-thomas]

Fixes #49.

This adds a RuntimeWarning for using floor division with integer dividers. Imo it is reasonable to add this warning to all supported distros since it is informing the user that something unexpected is going on. I don't see a use case where this behavior is actually intended. If existing code currently uses this it will result in a warning and the code an easily be updated.

Regarding the question if the class should always implement true division (for all or only newer ROS distros): Python has the same situation when doing integer division. Some might call it really bad default behavior but that's how Python does it. And there is a clean way (from __future__ import division) to explicitly use true division (which is the same across division operations across various types). Therefore I think it is reasonable to not change the current implementation.

A division by float already does a true division. And if the user requests an explicit float division that is what he wants. The fact that is performs the floor on seconds is reasonable imo. Updating the integer version of the function to return a more similar result is not necessary since the recommendation is to not use the function in the first place.

@ros/ros_team @eric-wieser Please review this carefully to be considered for merge and release as mentioned.

[eric-wieser]

First off, this is definitely an improvement over what the current behavour is, so thanks for that! I think a DeprecationWarning might be more suitable, since this should be aimed to be removed entirely at some future date.

Regarding the question if the class should always implement true division (for all or only newer ROS distros): Python has the same situation when doing integer division. Some might call it really bad default behavior but that's how Python does it.

This default behavour is at least meaningful - integer division is well-defined on scalars, and has "round to the nearest" semantics. As it is currently implemented on Durations, these semantics are broken.

A division by float already does a true division. And if the user requests an explicit [floor] division that is what he wants. The fact that is performs the floor on seconds is reasonable imo.

It's not terrible, but I think it violates Explicit is better than implicit. I'd also struggle to think of a use case anyway. This:

d = Duration(...)
f = float(...)
d_divrounded = d // f

Can be more clearly written and generically as

d = Duration(...)
f = float(...)
d_div = Duration() / f
# round to the nearest second
d_rounded = f_div // Duration(1) * Duration(1)

There's probably a good argument for a Duration.round to replace last line

Ultimately I go go either way on this one though,

Updating the integer version of the function to return a more similar result is not necessary since the recommendation is to not use the function in the first place.

With this choice, for a Duration d, d // 2.0 != d // 2. This is not true for any other type in python, so this should be corrected. If we're choosing to allow them at all, I think __floordiv__(float) must be consistent with __floordiv__(int). Therefore, I think the warning from __floordiv__ also needs to go. Leaving __div__ untouched for backwards compatibility seems reasonable.

[eric-wieser]

I think the best answer is just that we should exactly match the behavior of datetime.timedelta:

# python 3.5
>>> dt = timedelta(seconds=1, microseconds=500000)
>>> str(dt)
'0:00:01.500000'
>>> str(dt / 2)
'0:00:00.750000'
>>> str(dt // 2)
'0:00:00.750000'
>>> str(dt / 2.0)
'0:00:00.750000'
>>> str(dt // 2.0)
Traceback (most recent call last):
  File "<pyshell#48>", line 1, in <module>
    str(dt // 2.0)
TypeError: unsupported operand type(s) for //: 'datetime.timedelta' and 'float'
>>> dt // dt
1
>>> dt / dt
1.0

[dirk-thomas]

I don't think that a DeprecationWarning is helpful since it will by default not be shown anywhere. Imo the whole idea of the warning is to make people aware of the fact that they are using a floor division with a weird semantic.

We discussed this in our weekly meeting and will go with these warning for Indigo and Jade. For Kinetic the warning will be removed and the logic for integer division will be updated to behave like the float division. This will imply that Python 2 users not using the future import will use floor division (as with any other division).

I have also updated the warnings to mention the option to change the divisor to be a float.

[dirk-thomas]

@ros/ros_team @eric-wieser Please review that this can be merged and released as is into Indigo and Jade. I will create the Kinetic branch after that.

[wjwwood]

lgtm

[eric-wieser]

Using a float divisor here results in rounding to the nearest second, and does not result in "true division". This warning and/or the docstring should mention the round to the nearest second behavior, If you're not going to mention the correct behavior for floats, there's little point keeping __floordiv__ around at all in kinetic

[dirk-thomas]

I have removed the comment about the float divisor from __floordiv__. That only makes sense for __div__ where using floats actually makes a difference. Good catch.

[eric-wieser]

I was not aware that DeprecationWarning was ignored by default - good point

If you're set on floor division existing at all, then the kinetic migration path sounds good to me.

I don't think I'm really qualified to make a judgement on what can and can't be changed in released versions of ROS. Given that you'd prefer not to change it, this patch looks good to me (modulo my line--comment above)

[eric-wieser]

This seems to advise direct use of __truediv__ - calling magic methods directly is not recommended, as inevitably their semantics don't quite match the operator they implement. operator.truediv is a better substitute here, because that can correctly deal with NotImplemented and __rdiv__.

[dirk-thomas]

I updated it to mention operator.truediv instead. Thanks.

[eric-wieser]

I think there should be a "For division rounding to the nearest second, use a float divisor" here as well, unless you plan to scrap floordiv entirely in kinetic

[dirk-thomas]

I don't understand what the message is aiming for. floor is not equal to rounding to nearest seconds?

[dirk-thomas]

Nvm. Will updated the PR with another line added to the warning: https://github.com/ros/genpy/pull/58/files#diff-305939312869835e69b57abda55dd404R402

[eric-wieser]

Looks good to me

[dirk-thomas]

Thank you @eric-wieser for iterating on this.

[dirk-thomas]

And the followup PR for kinetic: #59.