-
Notifications
You must be signed in to change notification settings - Fork 7
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
BedDays
rework and slight performance gains
#1392
Draft
willGraham01
wants to merge
53
commits into
master
Choose a base branch
from
wgraham/beddays-rework
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… in missing functionality
…old copy of bed_days to copy across logging functionality
…efore BedDays is created
… nonexistent method
/run profiling |
Profiled run of the model failed ❌🆔 25740868490 |
… is used by several other modules
/run profiling |
Profiled run of the model failed ❌🆔 25933059488 |
/run profiling |
Profiled run of the model succeeded ✅🆔 25947662985 |
willGraham01
changed the title
Wgraham/beddays rework
Jun 10, 2024
BedDays
rework and slight performance gains
…ies, rather than instance of int
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR both attempts to reduce the time-footprint of the
BedDays
class on the overall simulation, and reworks the class so that it can be tested as a standalone object and does not contain any circular references to other instances.TODO:
tests/test_beddays.py
integration tests into either unit tests, or retain them as integration tests using the newBedDays
class. File can be deleted or incorporated intotest_bed_days.py
.tests/test_HealthSystem.py
file contains some references to now-removed attributes of theBedDays
class, which will need to be cleared up.Previous BedDays Tracking Method
The previous method of tracking bed days was:
bool
column in the population dataframe (hs_in_inpatient
) which flagged if a person was an inpatient (they are occupying, or are scheduled to occupy, a bed).Date
(hs_next_{first/last}_day_in_bed_{bed_type}
) columns in the population dataframe, which tracked when a person would next occupy/free a bed space.BedDays
class, each tracking 150 days into the future (with each day being stored as a row) and columns corresponding to the facility IDs of all facilities in the simulation. Entries in these dataframes were the number of beds available of the given type, on the given day, at the given facility.This necessitated an expensive update step to the population dataframe every day (to update the
hs_is_inpatient
column by a multi-column operation), and also meant that the trackers were continually having rows appended, deleted, and reordered. Furthermore, facilities with no beds still occupied places in said trackers, when they would never be assigned to.New Method of Tracking
The new method of tracking bed occupations introduces a dataclass
BedOccupancy
, and theBedDays
class now tracks a list of suchBedOccupancies
rather than tracking all facilities a fixed number of days into the future. This means:hs_next_{first/last}_day_in_bed_{bed_type}
columns in the population dataframe.hs_is_inpatient
column can be updated slightly quicker. We may even be able to avoid performing the update step entirely on days that it is not requested.BedDays
class can produce a forecast for the number of available beds, any number of days into the future, based on the current occupancies that are scheduled.Since
BedDays
is now tracking a list of individual objects, rather than using DataFrames, it would be worthwhile running the re-worked class in a large initial-population, large bed-capacity setting to check for any changes in performance due to the change in storage structure.Other Updates
BedDays
now contains unit tests intests/test_bed_days.py
, which can be conducted without tying the class to a simulation object.BedDaysFootprint
class has been added. It provides a convenient wrapper for creating footprints, converting them toBedOccupancies
, and also prevents modules from adding non-existent bed types to a requested footprint. The latter concern was previously being checked for in multiple places across the codebase, and these checks can (and have) been removed.Performance
edcced1
indicates that the footprint we were seeing from theBedDays
class is now gone. The proportional time spent inHeathSystemScheduler.apply
(which wraps all bed-days related code) has also fallen, implying the new functionality is indeed faster than the HEAD ofmaster
.