{"action":"created","issue":{"url":"https://api.github.com/repos/ros/genpy/issues/58","repository_url":"https://api.github.com/repos/ros/genpy","labels_url":"https://api.github.com/repos/ros/genpy/issues/58/labels{/name}","comments_url":"https://api.github.com/repos/ros/genpy/issues/58/comments","events_url":"https://api.github.com/repos/ros/genpy/issues/58/events","html_url":"https://github.com/ros/genpy/pull/58","id":149250704,"number":58,"title":"warn about using floor division of durations","user":{"login":"dirk-thomas","id":1335366,"avatar_url":"https://avatars.githubusercontent.com/u/1335366?v=3","gravatar_id":"","url":"https://api.github.com/users/dirk-thomas","html_url":"https://github.com/dirk-thomas","followers_url":"https://api.github.com/users/dirk-thomas/followers","following_url":"https://api.github.com/users/dirk-thomas/following{/other_user}","gists_url":"https://api.github.com/users/dirk-thomas/gists{/gist_id}","starred_url":"https://api.github.com/users/dirk-thomas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dirk-thomas/subscriptions","organizations_url":"https://api.github.com/users/dirk-thomas/orgs","repos_url":"https://api.github.com/users/dirk-thomas/repos","events_url":"https://api.github.com/users/dirk-thomas/events{/privacy}","received_events_url":"https://api.github.com/users/dirk-thomas/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-04-18T19:38:15Z","updated_at":"2016-04-18T20:07:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ros/genpy/pulls/58","html_url":"https://github.com/ros/genpy/pull/58","diff_url":"https://github.com/ros/genpy/pull/58.diff","patch_url":"https://github.com/ros/genpy/pull/58.patch"},"body":"Fixes #49.\r\n\r\nThis 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.\r\n\r\nRegarding 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.\r\n\r\nA 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.\r\n\r\n@ros/ros_team @eric-wieser Please review this carefully to be considered for merge and release as mentioned."},"comment":{"url":"https://api.github.com/repos/ros/genpy/issues/comments/211554095","html_url":"https://github.com/ros/genpy/pull/58#issuecomment-211554095","issue_url":"https://api.github.com/repos/ros/genpy/issues/58","id":211554095,"user":{"login":"eric-wieser","id":425260,"avatar_url":"https://avatars.githubusercontent.com/u/425260?v=3","gravatar_id":"","url":"https://api.github.com/users/eric-wieser","html_url":"https://github.com/eric-wieser","followers_url":"https://api.github.com/users/eric-wieser/followers","following_url":"https://api.github.com/users/eric-wieser/following{/other_user}","gists_url":"https://api.github.com/users/eric-wieser/gists{/gist_id}","starred_url":"https://api.github.com/users/eric-wieser/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eric-wieser/subscriptions","organizations_url":"https://api.github.com/users/eric-wieser/orgs","repos_url":"https://api.github.com/users/eric-wieser/repos","events_url":"https://api.github.com/users/eric-wieser/events{/privacy}","received_events_url":"https://api.github.com/users/eric-wieser/received_events","type":"User","site_admin":false},"created_at":"2016-04-18T20:07:36Z","updated_at":"2016-04-18T20:07:36Z","body":"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.\r\n\r\n> 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.\r\n\r\nThis 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.\r\n\r\n> 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.\r\n\r\nIt'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:\r\n\r\n```python\r\nd = Duration(...)\r\nf = float(...)\r\nd_divrounded = d // f\r\n```\r\n\r\nCan be more clearly written and generically as\r\n\r\n```\r\nd = Duration(...)\r\nf = float(...)\r\nd_div = Duration() / f\r\n# round to the nearest second\r\nd_rounded = f_div // Duration(1) * Duration(1)\r\n```\r\n\r\nThere's probably a good argument for a `Duration.round` to replace last line\r\n\r\nUltimately I go go either way on this one though,\r\n\r\n\r\n> 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.\r\n\r\nWith 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.\r\n"}}