SABS-R3-Epidemiology / epiabm

Epidemiological agent-based modelling packages in both python and C++. Published at: https://doi.org/10.5334/jors.449.
BSD 3-Clause "New" or "Revised" License
15 stars 5 forks source link

Bug Report - Multiple Workflows #268

Open KCGallagher opened 1 month ago

KCGallagher commented 1 month ago

Describe the bug

To Reproduce All workflows were ran with the default parameters provided - these should work 'out of the box'.

System information:

(Not believed to be relevant to issues described above).

Additional context Recommend additional change to include example workflows in integration testing - there are too many errors here that should have been picked up earlier, and the soft recommendation to check that all workflows run before submitting PRs is not being followed. This will be raised in a separate issue.

KCGallagher commented 1 month ago

@rccreswell Many of these issues result from missing parameters, corresponding to additional features in the new codebase. As a temporary fix I have added these values to all necessary parameter files, however this seems unsatisfactory - these files become lengthy and overcomplex through the inclusion of parameters that are not used, or take default values in the absence of more complex functionality (such as waning immunity) being included.

I want to recommend updating the parameters class to allow for hard-coding of default parameter values, that will be used if a parameter is not specified in the input file. For example, this would remove the requirement to specify rate multiplier parameters when waning immunity is not employed, as these values would default to unity.

The implementation of this change is non-trivial due to our singleton construction of the Parameters class (drawbacks of Python!) but I would be happy to look into this - what do you think?

rccreswell commented 1 month ago

@rccreswell Many of these issues result from missing parameters, corresponding to additional features in the new codebase. As a temporary fix I have added these values to all necessary parameter files, however this seems unsatisfactory - these files become lengthy and overcomplex through the inclusion of parameters that are not used, or take default values in the absence of more complex functionality (such as waning immunity) being included.

I want to recommend updating the parameters class to allow for hard-coding of default parameter values, that will be used if a parameter is not specified in the input file. For example, this would remove the requirement to specify rate multiplier parameters when waning immunity is not employed, as these values would default to unity.

The implementation of this change is non-trivial due to our singleton construction of the Parameters class (drawbacks of Python!) but I would be happy to look into this - what do you think?

Thanks @KCGallagher for your temporary fix in the previous Pull request and agreed on giving it some more thought.

I think a default parameters is sensible. One thing to consider is what info a user needs to record/know in order to recall exactly how their simulation results were obtained. If they keep the parameters json they used, will this one file completely specify the configured behavior of their simulation in a convenient way, or would they have to go digging into source code to understand the interpretation of unspecified default parameters?

Definitely still agreed on implementing these featues, and i may be out of date with the way this is handled, but may be worth giving some thought.