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

Pull HSI_Event class into its own submodule #1289

Merged
merged 13 commits into from
Mar 20, 2024

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Mar 5, 2024

This pull request moves the HSI_Event class (and related supporting classes) out of the healthsystem module and into its own submodule (hsi_event). This allows us to:

There are no functionality changes that are introduced by this PR.

Key changes

  • The HSI_Event class and related container classes have been moved into the src/tlo/methods/hsi_event.py file, to make them accessible without needing to import the entire healthcaresystem.py module.
    • This also lets us avoid circular imports that previously prevented type-hinting.
  • Internal logic of the HSI_Event class has been simplified slightly in places:
    • Class attributes are given typehints.
    • Avoids storing the parent Module, Simulation, and Population.
      • We now just store the Module.
      • The simulation can be accessed through a self.sim property to avoid storing it as a class attribute.
      • The healthcare system can also be accessed through a shortcut self.healthsystem property.
      • The target_is_alive property is used in place of tracking the population dataframe as a class attribute. We were only looking up one value from the dataframe, and otherwise not using it at all.
    • Avoids dynamically creating the return_item_codes_in_dict function every time the get_consumables method is run.
      • return_item_codes_in_dict is now a stand-alone method.
    • adjust_facility_level_to_merge_1b_and_2 has been made into a class method since it is not used outside of the HSI_Event class.
    • Typehints have been added to member functions and attributes where possible.

@willGraham01 willGraham01 changed the title Typehints for HSI_Event module Pull HSI_Event class into its own submodule Mar 8, 2024
@willGraham01
Copy link
Collaborator Author

willGraham01 commented Mar 12, 2024

/run profiling

This PR should be performance agnostic compared to master - it is just a refactoring.

@willGraham01 willGraham01 marked this pull request as ready for review March 12, 2024 10:39
@ucl-comment-bot
Copy link

Profiled run of the model succeeded ✅

🆔 22554782937
⏲️ 591 minutes
#️⃣ 1972781

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.

Thanks @willGraham01 these all looks like sensible changes to make. I have made a few small suggestions though none are vital.

Just so its recorded somewhere, Will and I discussed in person the changes to the imports of HSI_Event in other modules, which are not strictly necessary as HSI_Event is imported from the new tlo.methods.hsi_event module in tlo.methods.healthsystem and so could technically still be imported from there, which would keep the changes in this PR confined to the healthsystem and new hsi_event modules. Will pointed out changing the imports helps avoid circular imports between disease modules and healthsystem and should simplify importing HSI_Event to use just for type hinting purposes without needing any if TYPE_CHECKING blocks, which I felt was a good enough justification to outweigh the increased probability of needing to deal with some merge conflicts in disease modules on other branches when this is merged in (though as this only touches imports shouldn't be too much of a problem).

src/tlo/methods/hsi_event.py Outdated Show resolved Hide resolved
src/tlo/methods/hsi_event.py Outdated Show resolved Hide resolved
src/tlo/methods/hsi_event.py Outdated Show resolved Hide resolved
src/tlo/methods/hsi_event.py Outdated Show resolved Hide resolved
src/tlo/methods/hsi_event.py Outdated Show resolved Hide resolved
@willGraham01
Copy link
Collaborator Author

Have adapted the type-hinting to use from future and moved the stray method into the HSI_Event class to hide it. Re-requesting review in case there's anything else, otherwise will merge when (/if!) tests pass

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.

Thanks for doing the updates

@willGraham01 willGraham01 merged commit 864e5e1 into master Mar 20, 2024
56 checks passed
@willGraham01 willGraham01 deleted the wgraham/typehints-for-hsi-event branch March 20, 2024 14:31
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.

2 participants