-
Notifications
You must be signed in to change notification settings - Fork 3
Feat: Add column TotalPatients to CsAggregateMortalityAndHIVDeaths #577
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
base: dev
Are you sure you want to change the base?
Feat: Add column TotalPatients to CsAggregateMortalityAndHIVDeaths #577
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new column, ER diagram for CsAggregateMortalityAndHIVDeaths table changeserDiagram
CsAggregateMortalityAndHIVDeaths {
varchar FacilityName
int MFLCode
varchar PartnerName
varchar AgencyName
datetime ExitDate
varchar CohortYearMonth
varchar AgeGroup
int TotalPatients
int NoOfDeaths
int NoOfHIVDeaths
}
FACTART {
int PatientKey
int FacilityKey
int AgeGroupKey
}
Dimagegroup {
int AgeGroupKey
varchar Datimagegroup
}
PatientExits {
int PatientKey
int FacilityKey
datetime ExitDate
varchar CohortYearMonth
int Age
}
CsAggregateMortalityAndHIVDeaths ||--|| DimFacility : has
CsAggregateMortalityAndHIVDeaths ||--|| DimPartner : has
CsAggregateMortalityAndHIVDeaths ||--|| DimAgency : has
FACTART }o--|| Dimagegroup : has
PatientExits }o--|| FACTART : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @kevlanyo - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why
Age.Ageis used in the join toPatientExitsinstead ofAge.AgeGroupKey.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| LEFT OUTER JOIN NDWH.Dim.Dimpatient As Pat On Pat.Patientkey = PatientExits.Patientkey | ||
| LEFT OUTER JOIN NDWH.Dim.DimDate as Dateconfirmed ON Dateconfirmed.DateKey = Pat.Dateconfirmedhivpositivekey | ||
| WHERE PatientExits.dtDeadKey IS NOT NULL | ||
| ), PatientsData as ( |
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.
@kevlanyo I think this is related to the CS review discussions. I think we would need to start with the FactARTHistory to have patients cohort totals at each month and then join on died cohorts for each month.
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.
Exactly, @kevlanyo the FactART is a fact that represents data as of last completed month and therefore we will only have one denominator as opposed to the Fact ART History which we can get totals asof dates since we are plotting a trendline
@sourcery-ai review
Summary by Sourcery
New Features: