Skip to content

fixed btrdb pointwidth.from_timedelta issue#68

Merged
looselycoupled merged 8 commits intodevelopfrom
pointwidth-fix
Jun 19, 2020
Merged

fixed btrdb pointwidth.from_timedelta issue#68
looselycoupled merged 8 commits intodevelopfrom
pointwidth-fix

Conversation

@aorticweb
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread btrdb/utils/general.py
that the returned real duration is sufficient.
"""
return cls.from_nanoseconds(delta.total_seconds()*1e9)
return cls.from_nanoseconds(int(delta.total_seconds()*1e9))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will drop the fractional portion of the float. Is that really what we want to do here? As in, does this matter to the user or do we want to round up? I suppose I'm asking if there is a rare case where they are right on a boundary and we are chopping the value and they pop into the wrong pointwidth....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think in the vast majority of cases timedelta() * 1e9 will return a whole number as a float but in the rare cases where this timedelta() * 1e9 is a not a whole number, I think we should always round down since the user expects a point width as close to the length of the timedelta without going over it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds good to me!

Copy link
Copy Markdown
Member

@looselycoupled looselycoupled left a comment

Choose a reason for hiding this comment

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

approved!

@looselycoupled
Copy link
Copy Markdown
Member

@aorticweb actually this cannot be merged to the master branch as that is only for releases. Can you modify this PR to go into the develop branch?

@aorticweb aorticweb changed the base branch from master to develop June 19, 2020 14:51
@looselycoupled looselycoupled merged commit 1672463 into develop Jun 19, 2020
@looselycoupled looselycoupled deleted the pointwidth-fix branch June 19, 2020 15:57
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.

3 participants