Skip to content

Conversation

@zkoppert
Copy link
Member

@zkoppert zkoppert commented Oct 17, 2023

Proposed Changes

remove sub-seconds from stats. Addresses part of #144

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, or breaking

Signed-off-by: Zack Koppert <zkoppert@github.com>
@zkoppert zkoppert added the bug Something isn't working label Oct 17, 2023
Signed-off-by: Zack Koppert <zkoppert@github.com>
… for output

Signed-off-by: Zack Koppert <zkoppert@github.com>
@zkoppert zkoppert marked this pull request as ready for review October 17, 2023 23:44
Copy link
Contributor

@spier spier left a comment

Choose a reason for hiding this comment

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

Ran this locally, and it looks good.

Also looked through the code changes, and they LGTM.

Left one comment with a question.

continue
if label not in time_in_labels:
time_in_labels[label] = [issue.label_metrics[label]]
time_in_labels[label] = [issue.label_metrics[label].total_seconds()]
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of this PR is add numpy.round() and some cosmetic changes.

Just curious, why did you have to add the .total_seconds here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was definitely a mistake. In fact if the label is not in time_in_labels then I don't think we should be doing anything! I'll fix the total_seconds part here and remove the whole line in a separate PR with some testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I was wrong. 😄 This is needed. This part of the code stores the timedelta information for a given label and instead of storing it as a timedelta object, i'm choosing to store it as a number representing total_seconds so that round and averaging is easier. It is later converted back into a timedelta object after the math operations are complete.

@zkoppert zkoppert merged commit ecb3e4d into main Oct 19, 2023
@zkoppert zkoppert deleted the milliseconds branch October 19, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants