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

Bug Report - Multiple Workflows #268

Open
KCGallagher opened this issue Aug 4, 2024 · 2 comments · Fixed by #270
Open

Bug Report - Multiple Workflows #268

KCGallagher opened this issue Aug 4, 2024 · 2 comments · Fixed by #270
Assignees
Labels
bug Something isn't working python Changes related to pyEpiabm backend

Comments

@KCGallagher
Copy link
Contributor

Describe the bug

  • basic_simulation.py raises error AttributeError: '__Parameters' object has no attribute 'rate_multiplier_params'
  • spatial_simulation.py fails to read in a population from file, writing the same error to logs.
  • spatial_simulation.py also cannot use add_places method from ToyPopulaitonFactory.

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

System information:

  • OS: Windows
  • Python/ C++ Version: 3.9.12

(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 KCGallagher added bug Something isn't working python Changes related to pyEpiabm backend labels Aug 4, 2024
@KCGallagher KCGallagher self-assigned this Aug 4, 2024
@KCGallagher
Copy link
Contributor Author

@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?

KCGallagher added a commit that referenced this issue Aug 6, 2024
KCGallagher added a commit that referenced this issue Aug 6, 2024
KCGallagher added a commit that referenced this issue Aug 6, 2024
KCGallagher added a commit that referenced this issue Aug 6, 2024
@rccreswell rccreswell reopened this Aug 6, 2024
@rccreswell
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working python Changes related to pyEpiabm backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants