Skip to content
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

Remove population-wide HSI event in ChronicSyndrome module #1493

Conversation

marghe-molaro
Copy link
Collaborator

As raised in Issue #1260, our assumptions around how the healthsystem handles resource constraints does not allow for the use of Population-wide HSI events. Here we remove one such instance from the mock ChronicSyndrome module used by some of the HealthSystem tests.

Copy link
Collaborator

@matt-graham matt-graham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to #1260 - would the plan be to remove the logic in the health system scheduler for processing population scoped HSI events in a separate PR? It might also be good to add some logic to HSI_Event or PopulationScopeEventMixin in such a PR that raises an error when trying to subclass from both if so to ensure we fail loudly on trying to instantiate a population-scoped HSI event.

src/tlo/methods/chronicsyndrome.py Outdated Show resolved Hide resolved
tbhallett and others added 2 commits December 9, 2024 13:16
@tbhallett
Copy link
Collaborator

With regards to #1260 - would the plan be to remove the logic in the health system scheduler for processing population scoped HSI events in a separate PR? It might also be good to add some logic to HSI_Event or PopulationScopeEventMixin in such a PR that raises an error when trying to subclass from both if so to ensure we fail loudly on trying to instantiate a population-scoped HSI event.

Yes I think that's a good idea. @marghe-molaro do you want to do that, or shall I?

@marghe-molaro
Copy link
Collaborator Author

marghe-molaro commented Dec 13, 2024

Hi @tbhallett and @matt-graham,
The reason I didn't include that 'loud error' is that I'm a bit concerned that this might lead to failure during runtime if there are rare population-wide HSI events which are missed by tests. I think the safer option would be to remove the option for a pop-wide HSI completely, to make sure they cannot be used anywhere.

Sorry I misread your earlier message. yes I agree they should be completely removed.
I can take care of this @tbhallett.

@tbhallett
Copy link
Collaborator

Hi @tbhallett and @matt-graham, The reason I didn't include that 'loud error' is that I'm a bit concerned that this might lead to failure during runtime if there are rare population-wide HSI events which are missed by tests. I think the safer option would be to remove the option for a pop-wide HSI completely, to make sure they cannot be used anywhere.

What do you think?

I reckon we should take the plunge now (and in this PR) to remove the possibility of "Population-wide" HSI in the HealthSystem code entirely. (It clutters up the code, and going-forwards, it complicates things, and we don't want anyone to implement a new one!). We'll do tests to see if there is any other other population-HSI lurking. As @marghe-molaro proposes changes to accommodate the results of her emulation models etc, it seems a good time to make this change and set a cleaner baseline.

@marghe-molaro
Copy link
Collaborator Author

Just adding a note here that actually the original motivation for removing Pop-wide HSI events is that it would be difficult to enforce HCW time resource constraints on them.
Working on it now!

@tbhallett
Copy link
Collaborator

Just adding a note here that actually the original motivation for removing Pop-wide HSI events is that it would be difficult to enforce HCW time resource constraints on them. Working on it now!

Yes, that's quite right.

@tbhallett
Copy link
Collaborator

Closing this as this is now done, together with wider changes to remove Population level HSI in #1557

@tbhallett tbhallett closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants