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.

Sign up for GitHub

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

Jump to bottom

Viv3ckj/update descriptive stats measures #70

Merged
merged 7 commits into from
Nov 22, 2024
Merged

Viv3ckj/update descriptive stats measures #70

merged 7 commits into from
Nov 22, 2024

Conversation

viv3ckj
Copy link
Contributor

@viv3ckj viv3ckj commented Nov 22, 2024

Closes #68

milanwiedemann
milanwiedemann reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@milanwiedemann milanwiedemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of good ideas in here, I like the approach to wrap up some of our repeated queries in functions! I think the structure of the files, naming of functions and arguments, and the scope of each function could be improved. what do you thinK?

analysis/codelists.py Outdated Show resolved Hide resolved
analysis/codelists.py Outdated Show resolved Hide resolved
analysis/pf_codelist_functions.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file now does multiple things, not all of them are related to codelists. would this all fit into analysis/pf_variables_library.py or does it need it's own file? if so, what would be a better file name?

analysis/pf_codelist_functions.py Show resolved Hide resolved
analysis/pf_codelist_functions.py Outdated Show resolved Hide resolved
analysis/pf_codelist_functions.py Outdated Show resolved Hide resolved
milanwiedemann
milanwiedemann reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@milanwiedemann milanwiedemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much clearer now! I left some suggestions that will hopefully make it even easier to read

analysis/codelists.py Show resolved Hide resolved
analysis/measures_definition_pf_descriptive_stats.py Outdated Show resolved Hide resolved
analysis/measures_definition_pf_descriptive_stats.py Outdated Show resolved Hide resolved
analysis/measures_definition_pf_descriptive_stats.py Outdated Show resolved Hide resolved
analysis/measures_definition_pf_descriptive_stats.py Outdated Show resolved Hide resolved
analysis/measures_definition_pf_descriptive_stats.py Outdated Show resolved Hide resolved
milanwiedemann
milanwiedemann approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@milanwiedemann milanwiedemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good now!

@viv3ckj viv3ckj merged commit deb5c6f into main Nov 22, 2024
1 check passed
@viv3ckj viv3ckj deleted the viv3ckj/update-descriptive-stats-measures branch November 22, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milanwiedemann milanwiedemann milanwiedemann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix measures calculating descriptive statistics of Pharmacy First consultations
2 participants
@viv3ckj @milanwiedemann