-
Notifications
You must be signed in to change notification settings - Fork 13
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
Round Sprinty McBurndown's output to integers #1976
Conversation
@AlexanderStephensonUSDS let me know if you need help with the CI failures |
@coilysiren What do I need to fix for the Anchore Scan vulnerability check failure? |
@AlexanderStephensonUSDS if you look at the check, you'll see this It's referencing the current version of simpler-grants-gov/analytics/poetry.lock Lines 899 to 900 in 779201c
To fix the scan, you need to update $ poetry update |
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.
Hey @AlexanderStephensonUSDS thanks for finding and making these changes in the necessary places!
One small edit (which I should have made more clear in the ticket) we want to round the numbers to the nearest whole percentage point rather than simply flooring them to their integer value.
I left a few code suggestions that you should be able to accept, but can you update the other uses of int()
alone as well?
Yes. Makes sense.
Will make the requested changes.
On May 10, 2024, at 10:49 AM, Billy Daly ***@***.***> wrote:
@widal001 requested changes on this pull request.
Hey @AlexanderStephensonUSDS<https://github.com/AlexanderStephensonUSDS> thanks for finding and making these changes in the necessary places!
One small edit (which I should have made more clear in the ticket) we want to round the numbers to the nearest whole percentage point rather than simply flooring them to their integer value.
I left a few code suggestions that you should be able to accept, but can you update the other uses of int() alone as well?
________________________________
In analytics/src/analytics/metrics/burndown.py<#1976 (comment)>:
@@ -105,12 +105,12 @@ def get_stats(self) -> dict[str, Statistic]:
# get open and closed counts and percentages
total_opened = int(df["opened"].sum())
total_closed = int(df["closed"].sum())
- pct_closed = round(total_closed / total_opened * 100, 2)
+ pct_closed = int(total_closed / total_opened * 100)
I think we might actually want this:
⬇️ Suggested change
- pct_closed = int(total_closed / total_opened * 100)
+ pct_closed = int(round(total_closed / total_opened * 100, 0))
int() by itself just floors a float and we want to round to the nearest whole number
________________________________
In analytics/tests/metrics/test_burndown.py<#1976 (comment)>:
@@ -356,7 +356,7 @@ def test_include_issues_closed_after_sprint_end(self):
# validation
assert output.stats.get(self.TOTAL_CLOSED).value == 2
assert output.stats.get(self.TOTAL_OPENED).value == 3
- assert output.stats.get(self.PCT_CLOSED).value == 66.67 # rounded to 2 places
+ assert output.stats.get(self.PCT_CLOSED).value == 66 # rounded to 2 places
⬇️ Suggested change
- assert output.stats.get(self.PCT_CLOSED).value == 66 # rounded to 2 places
+ assert output.stats.get(self.PCT_CLOSED).value == 67 # rounded to nearest whole number
Changed this sounds it reflects the rounding to the nearest whole number
—
Reply to this email directly, view it on GitHub<#1976 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BADJYJMIWWOY7WMZFAZZZ5LZBTT7TAVCNFSM6AAAAABHPHBFWOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJQGM2DQMJVGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Co-authored-by: Billy Daly <[email protected]>
Co-authored-by: Billy Daly <[email protected]>
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.
LGTM!
I ran make sprint-reports-with-latest-data ACTION=post-results
and got the following results:
We'll tackle this in one of the other related issues in #2355 |
Summary
Fixes #1850
Time to review: 2 minutes
Changes proposed
Adds code to metrics to covert float percentages to whole integers.
Context for reviewers
Additional information
See Screenshot from local CLI.