Add monthly daly reporting to individual history tracker#1847
Add monthly daly reporting to individual history tracker#1847marghe-molaro wants to merge 9 commits into
Conversation
tbhallett
left a comment
tbhallett
left a comment
There was a problem hiding this comment.
All seems fine to me.
Two comments both thinking about speed, but do not feel very attached to either.
Happy to approve now
| data = { | ||
| person: {col: val for col, val in cols.items() if val != 0} | ||
| for person, cols in monthly_dalys_nonzero.items() | ||
| } | ||
| notifier.dispatch("healthburden.monthly_daly_report", data=data) |
There was a problem hiding this comment.
I'm wondering why you pack it into a dictionary here (two nested for-loops), and then unpack again (two more nested for-loops) when it goes to the function in the notifier? Could the pd.Dataframe be passed here, and then packed-into the eav format once?
There was a problem hiding this comment.
yes that's a good point; my naive guess was that the cost of reformatting non-zero entries into a dictionary is less than dispatching the entire dataframe which may contain a lot of zero entries*, but this may be incorrect. @tamuri happy for you to advise.
*This also a naive guess, based on the assumption that most individuals only have one or two conditions reported every months (meaning that all other columns would be zero for them), and that a single condition would typically affect <<20% (?) of the population.
There was a problem hiding this comment.
Tim is right - you don't need any filtering here. You're passing a reference, not a copy of the dataframe. You do need to ensure there is no editing of the dataframe in the receiver because it is used after the notification.
It's also how notifications are usually setup. You don't know how receivers need the information so send everything and then the receiver can do its own filtering (or not, as needed).
There was a problem hiding this comment.
An additional benefit is that you can remove the has_listeners check
|
Last commit now ensures that |
| for idx, row in monthly_dalys_nonzero.iterrows() | ||
| } | ||
|
|
||
| notifier.dispatch("healthburden.monthly_daly_report", data=data) |
There was a problem hiding this comment.
Use dash instead of underscores here, to match other listeners.
| notifier.dispatch("healthburden.monthly_daly_report", data=data) | |
| notifier.dispatch("healthburden.monthly-daly-report", data=data) |
tamuri
left a comment
tamuri
left a comment
There was a problem hiding this comment.
Mentioned some small changes, otherwise looks good.
3 participants
PR to ensure that the monthly dalys reported are included in the individual history tracker, broken down by cuase.
Note: In the healthburden module, we do not automatically dispatch the raw reported dalys dataframe in all instances, but first:
This should avoid unnecessarily dispatching large dataframes during runtime.