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

tamuri · 2024-07-22T09:25:55Z

src/tlo/simulation.py

@@ -44,7 +44,7 @@ class Simulation:
    """

    def __init__(self, *, start_date: Date, seed: int = None, log_config: dict = None,
-                 show_progress_bar=False):
+                 show_progress_bar=False, resourcefilepath = None):


Add str type hint to resourcefilepath.

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.

…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

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

thewati · 2024-10-02T12:18:38Z

Is there anyone who has an idea how I can solve this problem. Whenever I run scripts/hiv/batch_test_runs.py, I am
getting a file not found error. My path is getting mixed up with a path to ResourceFile_Demography_parameters.csv.
Can't pinpoint where these two paths are getting concatenated. The paths are C:\\Users\\User\\PycharmProjects\\TLOmodel\\src\\scripts\\hiv\\projections_jan2023\\batch_test_runs.py and demography\\ResourceFile_Demography_parameters.csv

…epath is set

tbhallett

Well done!!! This seems to do the job, and with a minimum if invasiveness! Thank you.

See a couple of comments/queries.

src/tlo/methods/epi.py

tbhallett · 2024-10-07T13:50:19Z

src/tlo/scenario.py

+        logger.info(key="message", data=f"Running draw {sample['draw_number']}, sample {sample['sample_number']}")
+
+        sim = Simulation(
+            start_date=self.scenario.start_date,
+            seed=sample["simulation_seed"],
+            log_config=log_config,
+            resourcefilepath=self.scenario.scenario_path)
+


confused about the need for these changes? Should they be on this PR?!?!

I was mostly following the Simulation objects to make changes to them wherever they are... If this change is not necessary for this PR, I can rollback

Yeah if it's not necessary for the PR, then I think it's best to roll it back.

src/tlo/scenario.py

tbhallett · 2024-10-07T13:51:07Z

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: str = None,


Suggested change

resourcefilepath: str = None,

resourcefilepath: Optional[str|Path] = None,

tbhallett

Thanks Wati and Joel. This all seems very good to me.

tbhallett · 2024-10-17T09:34:56Z

@tamuri and @matt-graham -- this looks ready to go, from my perspective

tbhallett · 2024-12-13T10:19:43Z

@jkumwenda -- please merge in master and resolve conflicts

@tamuri -- please can we merge this when done, so this doesn't keep getting out of date?

jkumwenda · 2024-12-13T10:50:18Z

Will fix that and merge

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

@jkumwenda --- please could you update this so we can merge it in? As we wait, it becomes out-of-date and requires more updates.

jkumwenda · 2025-01-08T13:22:44Z

On it Tim. Joel Kumwenda MSE, BSC, Cert, PDMPRO Software Consultant Cell(TZ) : (+255) 695 038 427 Cell(MW) : (+265) 999 371 088 Linked : https://www.linkedin.com/in/joel-kumwenda/ <http://www.linkedin.com/pub/joel-kumwenda/22/485/14>

…

On Wed, Jan 8, 2025 at 4:21 PM Tim Hallett ***@***.***> wrote: @jkumwenda <https://github.com/jkumwenda> --- please could you update this so we can merge it in? As we wait, it becomes out-of-date and requires more updates. — Reply to this email directly, view it on GitHub <#1410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGGIXLKRYH4ZXQH2PNEUDD2JUQ5NAVCNFSM6AAAAABKHATSPKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZXGY2TQOJUHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

# Conflicts: # src/scripts/epilepsy_analyses/analysis_epilepsy.py # src/tlo/methods/care_of_women_during_pregnancy.py # src/tlo/methods/contraception.py # src/tlo/methods/epi.py # src/tlo/methods/hiv.py # src/tlo/methods/hiv_tb_calibration.py # src/tlo/methods/labour.py # src/tlo/methods/malaria.py # src/tlo/methods/measles.py # src/tlo/methods/newborn_outcomes.py # src/tlo/methods/postnatal_supervisor.py # src/tlo/methods/pregnancy_supervisor.py # src/tlo/methods/schisto.py # src/tlo/methods/simplified_births.py # src/tlo/methods/tb.py

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?

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

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

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.

#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

thewati and others added 8 commits September 5, 2024 10:45

fix failing tests after merging

9559acf

Merge branch 'master' into jkumwenda/resource_file_path

c660f69

to be debugged

cdd90be

update simulation file

99c1c7c

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

7eb8e0a

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

update the simulation file

5c9a9ad

fix errors in scenario and tes_sim

4184764

update to scenario and tlo_parameters

ea16635

thewati added 4 commits October 3, 2024 09:31

fix test_module_dependencies, health_system, simulation

98873e5

fix scripts

fc5f5c6

fix hiv_tb_calibration file res access, and test to check resourcefil…

d5ed651

…epath is set

remove unexpected resourcefilepath in simulation

2cd182d

thewati requested a review from mnjowe October 7, 2024 07:06

tbhallett requested a review from tamuri October 7, 2024 13:41

tbhallett approved these changes Oct 7, 2024

View reviewed changes

mnjowe and others added 3 commits October 8, 2024 08:52

Merge branch 'master' into jkumwenda/resource_file_path

c8adb0c

review resourcefilepath in sim and rmv spaces in scenario

4e14f59

Rollback scenario.py

481ee3f

tbhallett approved these changes Oct 14, 2024

View reviewed changes

thewati and others added 3 commits January 9, 2025 17:01

Merge branch 'master' into jkumwenda/resource_file_path

26e8ad3

Fixed the contraception resourcepath in data_parameters()

8d2ac7c

mnjowe reviewed Jan 10, 2025

View reviewed changes

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

tamuri Jul 22, 2024

thewati commented Oct 2, 2024

tbhallett left a comment

tbhallett Oct 7, 2024

thewati Oct 8, 2024

tbhallett Oct 8, 2024

tbhallett Oct 7, 2024

tbhallett left a comment

tbhallett commented Oct 17, 2024

tbhallett commented Dec 13, 2024

jkumwenda commented Dec 13, 2024

tbhallett commented Jan 8, 2025

jkumwenda commented Jan 8, 2025 via email

mnjowe left a comment

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

mnjowe Jan 10, 2025

jkumwenda commented Jan 10, 2025

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

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

thewati commented Oct 2, 2024

tbhallett 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

tbhallett left a comment

Choose a reason for hiding this comment

tbhallett commented Oct 17, 2024

tbhallett commented Dec 13, 2024

jkumwenda commented Dec 13, 2024

tbhallett commented Jan 8, 2025

jkumwenda commented Jan 8, 2025 via email

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

jkumwenda commented Jan 10, 2025