UCL / TLOmodel

Epidemiology modelling framework for the Thanzi la Onse project
https://www.tlomodel.org/
MIT License
12 stars 5 forks source link

`do_at_generic_first_appt` and `do_at_generic_first_appt_emergency` should not be in core.Module (probably) #1350

Closed tamuri closed 4 months ago

tamuri commented 5 months ago

Re. the refactoring that happened in bd0a763. Some methods-submodule implementation details have leaked into the top-level part of the package. I'd argue this is not desirable, and we have tried to maintain a separation between what we considered the framework proper and the epi/modelling details which sit in the methods subpackage.

Some fixes may as simple as renaming e.g. in population module, a simple renaming of s/patient/person/ may be enough to make it general-purpose again. I guess it's called Patient because it was introduced in the context of HealthSystem improvements.

The more undersirable changes, IMO, are the changes in core where we've pulled in the HSI appointment methods into core.Module. Can we discuss some alternative ways to adopt Will's improvements but in a way that maintains SoC.

matt-graham commented 5 months ago

Agree with changing 'patient' to person (or individual) in PatientDetails structure added in population module, as I think it would be good to flag this can be used to get a readonly view of individual properties in all modules not just health system.

With regards to changes in core - if you wanted to keep the do_at_generic_first_appt and do_at_generic_first_appt_emergency methods out of Module then we could maybe instead add them to a subclass of Module that all the modules needing to deal with generic first appointments then derive from? I do think having these as methods with a standardised interface is a significant improvement over the previous situation of each module implementing its own logic in a slightly different way though.

willGraham01 commented 5 months ago

Subclassing Module will necessitate the need to filter for that particular subclass when running the generic appointments, since at present all the Modules registered with the simulation are bundled into a single dictionary.

But I like Matt's idea of a Module subclass regardless. Right now Module is used for both non-disease modules (Demography) and the disease ones, so having some distinction (and maybe two dicts per simulation to separate them) wouldn't be awful.

tamuri commented 5 months ago

A mixin is also a nice way to extend functionality for those classes that need it. AppointmentsMixin ?