-
Couldn't load subscription status.
- Fork 86
remove sub-seconds from stats #147
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
Conversation
Signed-off-by: Zack Koppert <zkoppert@github.com>
Signed-off-by: Zack Koppert <zkoppert@github.com>
… for output Signed-off-by: Zack Koppert <zkoppert@github.com>
There was a problem hiding this 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()] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Proposed Changes
remove sub-seconds from stats. Addresses part of #144
Readiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducingReviewer
bug,documentation,enhancement,infrastructure, orbreaking