-
Notifications
You must be signed in to change notification settings - Fork 16
Generate chain of events for individuals inc. for mni dataframe #1753
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: master
Are you sure you want to change the base?
Generate chain of events for individuals inc. for mni dataframe #1753
Conversation
Merged master in branch
…intained is generate_event_chains is None
…Log changes to logger.
…th, label what is only used for ddebugging and will be later removed
…ible to all modules. For now add person_ID to the dict of info printed as the outer dictionary key logging seems to have a problem.
…lysis file now collects all relevant info and prints them
…rected analysis file such as for small number of cases where the DALYs are not explicitly resolved the average DALYs are still computed correctly [skip ci]
- should only come from one place - same listener can listen for different notifications
|
To add following discussion at Q4:
|
The following population dataframe columns have object type i.e. the ones to check are copied/checked properly: |
…on, not df comparison
…ents logged following death in postprocessing
|
Hi @tamuri,
|
…github.com/UCL/TLOmodel into molaro/harvest-training-data-including-mni Merged master remotely, resync locally
|
Added features:
Note:
|
|
Now additionally tracking consumable, equipment, and bed days access for each individual |
tamuri
left a comment
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.
A few suggestions below...
Looking at the logs, I don't know how common is the following scenario: same individual, repeated same event on the same dat i.e. you would get a number of entries that were potentially of a different execution of the same event. If so, you could add an incrementing counter that gets logged as an identifier to group the entries in the EAV table. Might not be necessary if this doesn't happen.
src/tlo/methods/consumables.py
Outdated
| items_used=items_used, | ||
| ) | ||
|
|
||
| notifier.dispatch("consumables._request_consumables", data={'target' : target, 'event_name' : event_name, 'Item_Available': str(items_available),'Item_NotAvailable': str(items_not_available), 'Item_Used': str(items_used)}) |
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.
Change key to same format as elsewhere: consumables.post-request_consumables
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.
So unlike elsewhere, where the broadcasting happens before or after the named function is called, in this case the broadcasting occurs inside the _request_consumable fnc...so I opted to rename it consumables.on_request-consumables, but please let me know if you disagree
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.
I think it should still be called post-request_consumables. It is the state of things at the end of that function's block of code (like a postcondition), doesn't need to be in the caller.
src/tlo/methods/consumables.py
Outdated
| essential_item_codes: dict, | ||
| optional_item_codes: Optional[dict] = None, | ||
| to_log: bool = True, | ||
| to_broadcast: bool = True, |
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.
I think you don't need the to_broadcast argument here (I see that you check elsewhere where the individual tracker is registered) and simply use the existing `to_log? The notifier should always broadcast messages. Then the listeners register themselves to receive the message. If nothing is listening, there is no error. The obvious example is if a different module wants to listen for notifications.
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.
My reasoning for adding a check ahead of the broadcasting is not to avoid error, but rather to avoid having to create the items_available, items_not_available, and items_used dictionaries if they're not going to be used (through logging or broadcasting, though I think safest to disentangle the broadcasting from "to_log"). I have opted to check if the module is included here.
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.
I understand. It's still worth decoupling the notification from the module. Add the following to notify.py:
def has_listeners(self, notification_key):
"""
Check if there are any listeners registered for a specific notification.
:param notification_key: The identifier to check.
:return: True if there are listeners, False otherwise.
"""
return notification_key in self.listeners and len(self.listeners[notification_key]) > 0Then use it in the consumables at the point of notification i.e.
if notifier.has_listeners('consumables.post-request_consumables'):
# prepare the data
# ....
notifier.dispatch("consumables.post-request_consumables", data=....)Fix wrong dispatch identifier name in if statement Co-authored-by: Asif Tamuri <[email protected]>
This PR updates previous PR draft #1468
Now includes changes in the mother_and_newborn_info dataframe.