APP-410 Convert NBSSR00009 Bar Graph of Selected Disease by Month#3092
APP-410 Convert NBSSR00009 Bar Graph of Selected Disease by Month#3092krista-skylight wants to merge 37 commits intomainfrom
Conversation
Co-authored-by: Mary McGrath <m.c.mcgrath13@gmail.com>
Co-authored-by: Mary McGrath <m.c.mcgrath13@gmail.com>
This reverts commit 4a1f036. get rid of .gitignore change
…into kc/convert-nbssr00009 merge with main
…into kc/convert-nbssr00009 merge branches
krista-skylight
changed the title
…into kc/convert-nbssr00009 merge from main
…into kc/convert-nbssr00009 merge main
krista-skylight
requested review from
JordanGuinn
and removed request for
a team
| " COALESCE(state_cd, 'N/A') as state_cd,\n" | ||
| " COALESCE(state, 'N/A') as state,\n" | ||
| " COALESCE(county, 'N/A') as county,\n" | ||
| " COALESCE(cnty_cd, 'N/A') as cnty_cd,\n" |
There was a problem hiding this comment.
(thought, b): I know the SAS does coalescing with 'N/A' - I'm unconvinced we need to do this in SQL or if this is entirely equivalent as the SAS seems to be mapping '' => 'N/A', which isn't the same as NULL (or is it in SAS??) - what are your thoughts here?
There was a problem hiding this comment.
TO DO: get rid of coalesce, don't worry about null values
| ): | ||
| """Standard Report 09: Monthly Cases by Disease and State. | ||
|
|
||
| 'SR9: Bar Graph of Selected Disease by Month. Report demonstrates, using a |
There was a problem hiding this comment.
(sugg, nb): maybe add an "originally, " here since this is no longer a bar graph?
| # Format the time period string | ||
| time_period_str = f'{start_date} to {end_date}' if time_range else 'Last 12 Months' | ||
|
|
||
| header = 'SR9: Monthly Cases by Disease and State' |
There was a problem hiding this comment.
| header = 'SR9: Monthly Cases by Disease and State' | |
| header = 'SR9: Monthly Cases by Disease, County and State' |
maybe?
| assert result.content_type == 'table' | ||
|
|
||
| data = result.content.data | ||
| assert len(data) > 0 |
There was a problem hiding this comment.
(q, nb): given we know the data, can we make a stronger assertion here than > 0? 3 diseases, 3 states, 3 counties, 6 months -> 130ish rows?
There was a problem hiding this comment.
the specific 6 month time range I used in this test gave me 52 rows so I put the assertion at >50 right now. will that always remain the same as long as phc_demographic.yaml has the same random seed? If so, can I assume we're always going to keep it the same?
|
|
||
| # Verify data structure and basic sanity checks | ||
| for row in data: | ||
| # Find the row index for each column (don't assume order) |
There was a problem hiding this comment.
(q, nb): why not assume order since it is fixed?
There was a problem hiding this comment.
At first I assumed order and then left that note and chose to not assume order and list out all the names instead just to maybe make the test a little less brittle. Though I quickly discovered there was a downside when I went back to change the column names.
|
|
||
| # Month name should be 3 letters | ||
| month_name = row[col_index['month_name']] | ||
| assert len(month_name) in [3, 4] # Some months like 'Sept' might be 4 chars |
There was a problem hiding this comment.
(q, b): does SAS also have this "sometimtes 4" short name?
There was a problem hiding this comment.
Ooh I'm not sure about SAS. I put this in because I'm accustomed to python strftime abbreviating september to Sept. I googled and seems like MSSQL doesn't do that though so I think I can safely remove this.
krista-skylight
changed the title
| assert 'State(s):' in result.subheader | ||
| assert 'Disease(s):' in result.subheader | ||
| assert 'Time Period:' in result.subheader |
There was a problem hiding this comment.
(thought, b): this format looks different than SAS (and the other reports) - should we create a util that takes in a list of states and a time period formats the subheader?
(sugg, b): I don't see diseases in the original subheader - remove?
There was a problem hiding this comment.
created a util called gen_subheader within src.utils and rewrote to match the original subheader but with the pipe separator like in your example
|
|
||
| header = 'SR9: Monthly Cases by Disease and State' | ||
| subheader = ( | ||
| f'State(s): {", ".join(state_list) if state_list else "All"} | ' |
There was a problem hiding this comment.
if there are no states, is "All" a reasonable default? for SR2, I omit state if there are no states
There was a problem hiding this comment.
I ended up getting rid of this since this report should always have state and should error without one. I would have thought the default would be 'All' if we ever change it to not require selecting a state but I guess we can cross that bridge when we get there?
…SS-Modernization into kc/convert-nbssr00009 merge branches
…into kc/convert-nbssr00009 merge branches
|
2 participants
Checklist for adding a library:
apps/modernization-api/src/main/resources/db/report/execution/03_ODSE_Data_Report_Library_Init.sqlapps/modernization-api/src/main/resources/db/report/execution/librariesnamed<your library name>.sqlapps/modernization-api/src/main/resources/db/report-execution-changelog.yml. It should be added to the latestchangeSetsince the last release - this could require a newchangeSetif there isn't one since last releaseapps/report-execution/src/librariesnamed in lowercase, but generally following the naming convention of SAS (needs to be human recognizable as the same library)executefunction is the required method and its signature will (someday) be checked for validityapps/report-execution/tests/libraries/<your_library_here>pyfile following the conventions established for other librariessubset_sqlcan be assumed to be well tested by the modernization-api, so focus on any logic and additional joins/queries/analysis that is added in the library