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

Language clarification in extract_diagnostics #109

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Dec 9, 2024

Clarified:

  • "These diagnostics are then flagged if they exceed specific thresholds" makes it sound like the returned dataframe flags each individually (which it doesn't, hence suggest change of language)
  • "Exceed specific thresholds": sometimes it's if the value is below a threshold hence I suggest reword to "outside acceptable range"

Two possible refactors / improvements of this function could be:

  1. Return a flag for each diagnostic rather than for all of them jointly. Would it ever be useful to know in particular which diagnostic was violated (straight away, I know it could be recovered with some digging)
  2. Allowing setting of the thresholds. As a user I imagine this might be useful, like if certain conditions mean that diagnostics are over or under flagging. I can see here it's a bit annoying to do and would probably have to be like a user input list of named values.

Copy link

github-actions bot commented Dec 9, 2024

Thank you for your contribution @zsusswein 🚀! Your pkgdown-site is ready for download 👉 here 👈!
(The artifact expires on 2024-12-17T15:06:58Z. You can re-generate it by re-running the workflow here.)

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

@zsusswein
Copy link
Collaborator

Do you want to rebuild the docs with roxygen2::roxygenize() or should I?

@athowes
Copy link
Collaborator Author

athowes commented Dec 10, 2024

Do you want to rebuild the docs with roxygen2::roxygenize() or should I?

I will.

@athowes
Copy link
Collaborator Author

athowes commented Dec 10, 2024

Note that it's odd that CI didn't fail (I think?) for the above. I would have thought the website would complain that the docs were not up-to-date? If something missing suggest to add.

@zsusswein
Copy link
Collaborator

Yeah we need to do this CDCgov/cfa-gam-rt#57

Clarified:

* "These diagnostics are then flagged if they exceed specific thresholds" makes it sound like the returned dataframe flags each __individually__ (which it doesn't, hence suggest change of language)
* "Exceed specific thresholds": sometimes it's if the value is below a threshold hence I suggest reword to "outside acceptable range"

Possible refactors / improvements of this function could be:

<details>

1. Return a flag for each diagnostic rather than for all of them jointly. Would it ever be useful to know in particular which diagnostic was violated (straight away, I know it could be recovered with some digging)
2. Allowing setting of the thresholds. As a user I imagine this might be useful, like if certain conditions mean that diagnostics are over or under flagging. I can see here it's a bit annoying to do and would probably have to be like a user input list of named values.
@zsusswein zsusswein enabled auto-merge (squash) December 10, 2024 15:05
@zsusswein zsusswein disabled auto-merge December 11, 2024 13:39
@zsusswein zsusswein merged commit 66ac3ea into main Dec 11, 2024
10 of 11 checks passed
@zsusswein zsusswein deleted the athowes-patch-2 branch December 11, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants