Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add utilities to deal with time and duration #103

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

mhidalgo-bdai
Copy link
Contributor

Precisely what the title says. Use of Time and Duration representations in Python, rclpy, rosbag2, and our own codebase is not particularly homogeneous. This patch introduces minimal conventions to simplify API implementation.

Copy link
Contributor

@khughes-bdai khughes-bdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- just left a minor comment

def as_proper_duration(duration: Union[int, float, timedelta, Duration]) -> Duration:
"""Return `duration` as a proper Duration object.

Note that for scalar durations the convention is that floating point durations

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add a proper docstring with input specification including units?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not exactly sure what you mean by input specification.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, something like the following:

Suggested change
Note that for scalar durations the convention is that floating point durations
"""
Convert various duration inputs into a standardized Duration object.
Parameters:
duration (Union[int, float, timedelta, Duration]): The duration to be converted.
- If an int is provided, it is assumed to be the duration in seconds.
- If a float is provided, it is assumed to be the duration in seconds.
- If a timedelta is provided, it is converted to the appropriate Duration object.
- If a Duration object is provided, it is returned as is.
Returns:
Duration: A standardized Duration object representing the input duration.
Raises:
ValueError: If the input duration is not of a supported type.
Example:
>>> as_proper_duration(3600)
<Duration: 3600 seconds>
>>> as_proper_duration(2.5)
<Duration: 2.5 seconds>
>>> as_proper_duration(timedelta(hours=1))
<Duration: 3600 seconds>
>>> as_proper_duration(Duration(3600))
<Duration: 3600 seconds>
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! The first example is wrong. Katie's right. The current behavior is going to be confusing. I'll standardize to SI units and include this docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c3262c8. I purposely opted out from duplicating type annotations in docstrings.

Copy link

@robodreamer robodreamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment, but looks nice.

@mhidalgo-bdai
Copy link
Contributor Author

mhidalgo-bdai commented Jun 11, 2024

It looks like #96 wasn't enough. The log forwarding test keeps flaking -.-

Copy link

@robodreamer robodreamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments!

@mhidalgo-bdai
Copy link
Contributor Author

Success!

@mhidalgo-bdai mhidalgo-bdai merged commit dbb1bab into main Jun 12, 2024
3 checks passed
@mhidalgo-bdai mhidalgo-bdai deleted the mhidalgo-bdai/time-utilities branch June 12, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants