HHS / simpler-grants-gov

https://simpler.grants.gov
Other
42 stars 11 forks source link

Round Sprinty McBurndown's output to integers #1976

Open AlexanderStephensonUSDS opened 4 months ago

AlexanderStephensonUSDS commented 4 months ago

Summary

Fixes #1850

Time to review: 2 minutes

Changes proposed

Adds code to metrics to covert float percentages to whole integers.

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. Explain how the changes were verified.

Additional information

See Screenshot from local CLI. burndown burnup percent_deliver

coilysiren commented 4 months ago

@AlexanderStephensonUSDS let me know if you need help with the CI failures

AlexanderStephensonUSDS commented 4 months ago

@coilysiren What do I need to fix for the Anchore Scan vulnerability check failure?

coilysiren commented 4 months ago

@AlexanderStephensonUSDS if you look at the check, you'll see this

image

It's referencing the current version of Jinja2, which is being installed here: https://github.com/HHS/simpler-grants-gov/blob/779201c4c6c5d95a81cd60245d1103d785629c3f/analytics/poetry.lock#L899-L900

To fix the scan, you need to update Jinja2. You can do that with the following command:

$ poetry update
AlexanderStephensonUSDS commented 4 months ago

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 @AlexanderStephensonUSDShttps://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.pyhttps://github.com/HHS/simpler-grants-gov/pull/1976#discussion_r1596916888:

@@ -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

int() by itself just floors a float and we want to round to the nearest whole number


In analytics/tests/metrics/test_burndown.pyhttps://github.com/HHS/simpler-grants-gov/pull/1976#discussion_r1596919569:

@@ -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

Changed this sounds it reflects the rounding to the nearest whole number

— Reply to this email directly, view it on GitHubhttps://github.com/HHS/simpler-grants-gov/pull/1976#pullrequestreview-2050348150, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BADJYJMIWWOY7WMZFAZZZ5LZBTT7TAVCNFSM6AAAAABHPHBFWOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJQGM2DQMJVGA. You are receiving this because you were mentioned.Message ID: @.***>