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

remove sub-seconds from stats #147

Merged
merged 5 commits into from
Oct 19, 2023
Merged

remove sub-seconds from stats #147

merged 5 commits into from
Oct 19, 2023

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

@zkoppert zkoppert added the bug Something isn't working label Oct 17, 2023
@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.

@@ -101,17 +101,25 @@ def get_stats_time_in_labels(
if issue.label_metrics[label] is None:
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
5 checks passed
@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.

2 participants