Resource file path from simulation #1410

jkumwenda · 2024-07-02T08:17:52Z

Created resource file path function and calling it from different modules.

… path from simulation

src/tlo/simulation.py

tamuri · 2024-07-22T09:28:18Z

src/tlo/simulation.py

@@ -80,6 +81,7 @@ def __init__(self, *, start_date: Date, seed: int = None, log_config: dict = Non
            data=f'Simulation RNG {seed_from} entropy = {self._seed_seq.entropy}'
        )
        self.rng = np.random.RandomState(np.random.MT19937(self._seed_seq))
+        self.resourcefilepath = resourcefilepath


Here, we could convert and store Path type and check that path exists.

I think this hasn't;t been done yet (the check that the path exists)

…alysis rti_deaths, re-worked test_rti

…lthcareseeking

…tate cancer, fixed test equipment and dxmanager

…e path from simulation.py.

…on.py.

…t.py and breast_cancer.py method updated for resource file path from simulation.py

…t.py isort the import to fix incorrectly sorted error

…vert to path in simulation

mnjowe

Thank you, Wati and Joel, for the excellent work on this PR. Below are my comments, which may seem many but primarily revolve around the following key points:

I suggest initialising resourcefilepath as a path object in both the analysis scripts and test files. This change will help eliminate repetitive code currently arising from creating a path object from resourcefilepath in each disease module.
I suggest reverting the changes made to the simulation end_date and population sizes in some of the analysis scripts.
I suggest removing str option for resourcefilepath argument in Simulation object.
I suggest making resourcefilepath argument in the read_parameters section optional to improve readability
I suggest removing the condition to check if resourcefilepath is empty in utils. There may be a more efficient way to handle this check.

For changing resourcefilepath from str to path, I couldn't provide a comment on every affected line. However, if you agree that it should be declared as a path object (rather than a string), you can apply this change consistently across all affected areas. Similarly, if you agree with making resourcefilepath in read_parameters section optional, you can apply this adjustment to all relevant sections.

Once again, thank you for the great work on this PR!

mnjowe · 2025-01-10T06:16:20Z

src/scripts/cardio_metabolic_disorders_analyses/cardiometabolicdisorders_plots.py

+    end_date = Date(2011, 12, 31)
+    popsize = 5000


Did you make these changes just to make the script run faster? if yes can you now please revert the changes?

Yes. Will revert the changes

mnjowe · 2025-01-10T06:19:39Z

src/scripts/copd_analyses/analysis_copd_prevalence_and_deaths.py

@@ -348,7 +348,7 @@ def plot_modal_gbd_deaths_by_age_group(self):
 start_date = Date(2010, 1, 1)
 end_date = Date(2030, 1, 1)

-resourcefilepath = Path("./resources")  # Path to resource files
+resourcefilepath = './resources'  # Path to resource files


Any reason why you're changing from Path to string here?

mnjowe · 2025-01-10T06:25:59Z

src/scripts/depression_analyses/analyses_depression_run_me.py

+    end_date = Date(2011, 7, 1)
+    pop_size = 1000


same as above, if the intention was to make this run faster, revert the changes

mnjowe · 2025-01-10T06:31:24Z

src/scripts/depression_analyses/analyses_depression_run_me.py


    # Path to the resource files used by the disease and intervention methods
-    resources = "./resources"
+    resourcefilepath = "./resources"


can we standardise path here i.e. making it to resourcefilepath = Path("./resources") and read it in read_parameters as read_csv_files(resourcefilepath / resourcefile_folder_name). I feel this will be good as we will initialise path once rather than each module initialising it.

mnjowe · 2025-01-10T06:32:22Z

src/scripts/diarrhoea_analyses/analysis_diarrhoea_with_and_without_treatment.py

@@ -25,7 +25,7 @@
 # %%


-resourcefilepath = Path("./resources")
+resourcefilepath = './resources'


same here. why changing? I feel like it will be good to initialise path here rather than in the module

We overlooked this. Thanks for catching it

mnjowe · 2025-01-10T07:38:37Z

src/tlo/methods/breast_cancer.py

@@ -192,12 +191,12 @@ def __init__(self, name=None, resourcefilepath=None):
        )
    }

-    def read_parameters(self, data_folder):
+    def read_parameters(self, resourcefilepath=None):


make it optional?

mnjowe · 2025-01-10T07:39:53Z

src/tlo/methods/breast_cancer.py

        """Setup parameters used by the module, now including disability weights"""

        # Update parameters from the resourcefile
        self.load_parameters_from_dataframe(
-            pd.read_excel(Path(self.resourcefilepath) / "ResourceFile_Breast_Cancer.xlsx",
+            pd.read_excel(Path(resourcefilepath) / "ResourceFile_Breast_Cancer.xlsx",


could be good if you could have initialised resourcefilepath as path object and avoid creating it here

mnjowe · 2025-01-10T07:40:34Z

src/tlo/methods/cardio_metabolic_disorders.py

@@ -256,7 +255,7 @@ def __init__(self, name=None, resourcefilepath=None, do_log_df: bool = False, do
        self.lms_event_death = dict()
        self.lms_event_symptoms = dict()

-    def read_parameters(self, data_folder):
+    def read_parameters(self, resourcefilepath=None):


make it optional?

mnjowe · 2025-01-10T07:41:56Z

src/tlo/methods/cardio_metabolic_disorders.py

@@ -273,7 +272,7 @@ def read_parameters(self, data_folder):
        ResourceFile_cmd_events_hsi.xlsx  = HSI parameters for events

        """
-        cmd_path = Path(self.resourcefilepath) / "cmd"
+        cmd_path = Path(resourcefilepath) / "cmd"


resourcefilepath could be passed as a path object already and avoid creating it here

mnjowe · 2025-01-10T07:45:04Z

src/tlo/simulation.py

    def __init__(
        self,
        *,
        start_date: Date,
        seed: Optional[int] = None,
        log_config: Optional[dict] = None,
        show_progress_bar: bool = False,
-        resourcefilepath: Optional[Path] = None,
+        resourcefilepath: Optional[str | Path] = None,


this should only take Path. I think string option should be removed

agreed about query of whether we think string is ok here -- and I wasn't sure if the whole argument should be Optional.

I see below that a string would be OK as line 112 wraps this in Path().

I also see that the comment on line 93 explains that not giving a parameter is ok.

I don't know why that would be mean, but there might be a reason.

Suggested change

resourcefilepath: Optional[str | Path] = None,

resourcefilepath: Path,

jkumwenda · 2025-01-10T09:06:29Z

Thank you, Wati and Joel, for the excellent work on this PR. Below are my comments, which may seem many but primarily revolve around the following key points:

I suggest initialising resourcefilepath as a path object in both the analysis scripts and test files. This change will help eliminate repetitive code currently arising from creating a path object from resourcefilepath in each disease module.

I suggest reverting the changes made to the simulation end_date and population sizes in some of the analysis scripts.

I suggest removing str option for resourcefilepath argument in Simulation object.

I suggest making resourcefilepath argument in the read_parameters section optional to improve readability

I suggest removing the condition to check if resourcefilepath is empty in utils. There may be a more efficient way to handle this check.

For changing resourcefilepath from str to path, I couldn't provide a comment on every affected line. However, if you agree that it should be declared as a path object (rather than a string), you can apply this change consistently across all affected areas. Similarly, if you agree with making resourcefilepath in read_parameters section optional, you can apply this adjustment to all relevant sections.

Once again, thank you for the great work on this PR!

Thanks for these comments, we will review and provide feedback line by line.

…to jkumwenda/resource_file_path

…th("./resources") in scripts files and updated methods read parameters to def read_parameters(self, resourcefilepath: Optional[Path] = None): helps with single initialisation across the methods

# Conflicts: # src/tlo/methods/alri.py # src/tlo/methods/depression.py # src/tlo/methods/diarrhoea.py # src/tlo/methods/epilepsy.py # src/tlo/methods/rti.py

…ontraception'].resourcefilepath)

…rent activ resource file in the HIV resource folder

mnjowe · 2025-01-15T09:46:47Z

src/tlo/methods/hiv_tb_calibration.py

+        data_hiv_mphia_inc = xls["MPHIA_incidence2020"]
+        data_hiv_mphia_prev = xls["MPHIA_prevalence_art2020"]


@tdm32, can you confirm this change is necessary. I agree with Joel, We don't have MPHIA_incidence2015 and MPHIA_prevalence_art2015 in the HIV resourcefiles folder.

Yes, this script originally used the MPHIA 2015 estimates, but would now use the 2020 estimates. The worksheets were renamed, so MPHIA_incidence2015 and MPHIA_prevalence_art2015 no longer exist. Thank you.

Thanks Tara

…s(self, resourcefilepath: Optional[Path] = None): parameter_dataframe = read_csv_files(resourcefilepath /

…sourcefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

…cefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

…to jkumwenda/resource_file_path # Conflicts: # src/tlo/methods/scenario_switcher.py

# Conflicts: # src/tlo/analysis/utils.py # src/tlo/methods/bladder_cancer.py # src/tlo/methods/breast_cancer.py # src/tlo/methods/enhanced_lifestyle.py # src/tlo/methods/oesophagealcancer.py # src/tlo/methods/other_adult_cancers.py # src/tlo/methods/prostate_cancer.py # src/tlo/methods/stunting.py

jkumwenda · 2025-01-21T11:08:55Z

@tbhallett please review and merge in master if all is good. I have addressed all comments from @mnjowe

tbhallett · 2025-01-21T13:58:08Z

src/tlo/simulation.py

    def __init__(
        self,
        *,
        start_date: Date,
        seed: Optional[int] = None,
        log_config: Optional[dict] = None,
        show_progress_bar: bool = False,
-        resourcefilepath: Optional[Path] = None,
+        resourcefilepath: Optional[str | Path] = None,


agreed about query of whether we think string is ok here -- and I wasn't sure if the whole argument should be Optional.

I see below that a string would be OK as line 112 wraps this in Path().

I also see that the comment on line 93 explains that not giving a parameter is ok.

I don't know why that would be mean, but there might be a reason.

Suggested change

resourcefilepath: Optional[str | Path] = None,

resourcefilepath: Path,

tbhallett · 2025-01-21T14:54:54Z

src/tlo/methods/schisto.py

@@ -119,11 +118,11 @@ def __init__(self, name=None, resourcefilepath=None, mda_execute=True):
                s.loc[(s.index >= low_limit) & (s.index <= high_limit)] = name
        self.age_group_mapper = s.to_dict()

-    def read_parameters(self, data_folder):
+    def read_parameters(self, resourcefilepath: Optional[Path] = None):


here - and elsewhere -- why do we say this is optional?

This is the correct type hint for an argument that's allowed to be None.

Re. resourcefilepath: Path, suggestion - we're always specifying a resource path for a Simulation (I don't think any of our modules work without one!), so I don't think it'd be very disruptive to remove the default value.

tbhallett · 2025-01-21T15:00:13Z

src/tlo/methods/scenario_switcher.py

@@ -52,7 +51,7 @@ def __init__(self, name=None, resourcefilepath=None):

    PROPERTIES = {}

-    def read_parameters(self, data_folder):
+    def read_parameters(self, *args):


this signature is different to that for all the other modules - but I think it should be the same.

tbhallett · 2025-01-21T16:46:34Z

Great work, @jkumwenda --- so glad this is working now.

My comments will mostly be for @tamuri, I think. I am not sure myself what the right thing to do would be, so have commented where I would instinctively have done something different.

#Read function and enhanced lifestyle

6cbd941

jkumwenda requested review from tamuri, matt-graham and tbhallett July 2, 2024 08:17

jkumwenda assigned jkumwenda and thewati Jul 2, 2024

jkumwenda linked an issue Jul 2, 2024 that may be closed by this pull request

Get resource file path from simulation #1375

Open

5 tasks

jkumwenda added 2 commits July 4, 2024 11:23

Updated the resource file path and register function in simulation.py

041ab47

Implemented resourcefilepath for Disease modules to get resource file…

9129f4e

… path from simulation

tamuri requested changes Jul 22, 2024

View reviewed changes

thewati and others added 20 commits July 30, 2024 15:50

RTI and associated modules

3f07713

Schisto and associated modules

57870a3

analysis_malaria and associated files

fc66ff1

analysis file of hiv and associated file, fixed test_schisto

8815e89

epi analyses and associated files, removed unused import in analysis rti

3dbe968

diarhhhoea analyses and associated files, removed unused import in an…

414dbbb

…alysis rti_deaths, re-worked test_rti

analyses_copd_prev and associated files

b813ac3

alri analyses and associated files, fixed test_enhanced_lifestyle.py

73bac49

prostate_cancer_analyses_logging and associated files, fixed test_hea…

e64735e

…lthcareseeking

other_adult_cancers_analyses_logging and associated files, isort pros…

64a0c7b

…tate cancer, fixed test equipment and dxmanager

analysis_measles.py measles.py test_measles.py fixed for resource fil…

6e6be58

…e path from simulation.py.

analysis_measles_deaths.py fixed for resource file path from simulati…

166ad35

…on.py.

analysis_measles.py removed unused from pathlib import Path .

78ad0be

breast_cancer_analyses_logging.py calc_5y_survival_following_treatmen…

690a1f7

…t.py and breast_cancer.py method updated for resource file path from simulation.py

breast_cancer_analyses_logging.py calc_5y_survival_following_treatmen…

0c9dabc

…t.py and breast_cancer.py method updated for resource file path from simulation.py

breast_cancer_analyses_logging.py calc_5y_survival_following_treatmen…

ae6309b

…t.py isort the import to fix incorrectly sorted error

bladder_cancer_analyses_single_run, calc_5y and associated files, con…

0621041

…vert to path in simulation

fix resourcefilepath for test_beddays and test_cons

16e026d

fix test_demography and associated files, fixed fullmodel file

1cb1b88

fix test utils, labour and newborn_outcomes

e5335df

Fixed the contraception resourcepath in data_parameters()

0798c6f

mnjowe reviewed Jan 10, 2025

View reviewed changes

jkumwenda added 7 commits January 14, 2025 11:23

Merge remote-tracking branch 'origin/jkumwenda/resource_file_path' in…

b1c079c

…to jkumwenda/resource_file_path

Malaria methods resource file path

c3545ad

Updated resourcefilepath = './resources' to the resourcefilepath = Pa…

d5bf63d

…th("./resources") in scripts files and updated methods read parameters to def read_parameters(self, resourcefilepath: Optional[Path] = None): helps with single initialisation across the methods

Merge branch 'master' into jkumwenda/resource_file_path

2f93e27

# Conflicts: # src/tlo/methods/alri.py # src/tlo/methods/depression.py # src/tlo/methods/diarrhoea.py # src/tlo/methods/epilepsy.py # src/tlo/methods/rti.py

update the resource path in contraception.py Path(self.sim.modules['C…

2635ea4

…ontraception'].resourcefilepath)

update the resource path in contraception.py Path(self.sim.modules['C…

c50606b

…ontraception'].resourcefilepath)

upated the HIV Calibration xls["MPHIA_incidence2015"] to 2020 the cur…

b1029c9

…rent activ resource file in the HIV resource folder

mnjowe reviewed Jan 15, 2025

View reviewed changes

jkumwenda added 14 commits January 15, 2025 13:37

import Path in chronicsyndrome.py

584ed45

fixed resource file path read_parameters call data def read_parameter…

342cca6

…s(self, resourcefilepath: Optional[Path] = None): parameter_dataframe = read_csv_files(resourcefilepath /

Imports are correctly sorted and/or formatted.

936f42a

Imports are correctly sorted and/or formatted.

04b00a4

Imports are correctly sorted and/or formatted.

740ce3f

Imports are correctly sorted and/or formatted.

11eef01

scenario_switcher.py does not require resourcefilepath, remmove if re…

d049a0c

…sourcefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

scenario_switcher.py does not require resourcefilepath, remmove if re…

16edc89

…sourcefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

scenario_switcher.py does not require resourcefilepath, remmove if re…

dc77227

…sourcefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

scenario_switcher.py does require resourcefilepath, remmove if resour…

484baa7

…cefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

scenario_switcher.py does require resourcefilepath, remmove if resour…

23e2ab7

…cefilepath is None: resourcefilepath = get_root_path() / 'resources' from utils.py.

Merge remote-tracking branch 'origin/jkumwenda/resource_file_path' in…

775973c

…to jkumwenda/resource_file_path # Conflicts: # src/tlo/methods/scenario_switcher.py

scenario_switcher.py resource file path update in read_parameters

821912e

tbhallett reviewed Jan 21, 2025

View reviewed changes

thewati and others added 2 commits January 27, 2025 15:21

scenario switcher suggestion

e7dfdec

Merge branch 'master' into jkumwenda/resource_file_path

b6f3386

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource file path from simulation #1410

Resource file path from simulation #1410

jkumwenda commented Jul 2, 2024

tamuri Jul 22, 2024

tbhallett Jan 21, 2025

mnjowe left a comment

mnjowe Jan 10, 2025

thewati Jan 13, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

thewati Jan 13, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

tbhallett Jan 21, 2025 •

edited

Loading

jkumwenda commented Jan 10, 2025

mnjowe Jan 15, 2025

tdm32 Jan 15, 2025

mnjowe Jan 15, 2025

jkumwenda commented Jan 21, 2025 •

edited

Loading

tbhallett Jan 21, 2025 •

edited

Loading

tbhallett Jan 21, 2025

tamuri Jan 22, 2025

tamuri Jan 22, 2025

tbhallett Jan 21, 2025

tbhallett commented Jan 21, 2025

	resourcefilepath: Optional[str \| Path] = None,
	resourcefilepath: Path,

		data_hiv_mphia_inc = xls["MPHIA_incidence2020"]
		data_hiv_mphia_prev = xls["MPHIA_prevalence_art2020"]

Resource file path from simulation #1410

Are you sure you want to change the base?

Resource file path from simulation #1410

Conversation

jkumwenda commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnjowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbhallett Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

jkumwenda commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkumwenda commented Jan 21, 2025 • edited Loading

tbhallett Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbhallett commented Jan 21, 2025

tbhallett Jan 21, 2025 •

edited

Loading

jkumwenda commented Jan 21, 2025 •

edited

Loading

tbhallett Jan 21, 2025 •

edited

Loading