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

Ensure every instance of Simulation class has dataset, fix check_macro_cache #266

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

anth-volk
Copy link
Contributor

Fixes #262.

Previously, the Simulation class took an optional argument defaulted to None for a dataset parameter, which was used only when extending Simulation into classes like MicroSimulation and was always created with a default value of None if not provided. If a dataset was provided, it appears to have correctly integrated that into the Simulation instance. However, a dataset is not always provided, particularly by pe-core's test runner, which does not provide one.

This creates a confusing behavior whereby the dataset argument is always present, but an individual Simulation instance's self.dataset property is only created when dataset is not None. Furthermore, there are times when it appears self.dataset is created, but made equal to None. Furthermore, it's fairly easy to confuse the self.dataset instance attribute with the self.datasets class attribute, which is always present.

In order to disambiguate, this PR always instantiates a self.dataset instance attribute based upon the dataset arg, which continues to have a default value of None. Relying upon the expectation that self.dataset is always defined, it modifies the self.check_macro_cache method, but also adds a safety valve check to properly return False if self.dataset does not exist, just in case this PR missed some behavior somewhere.

@MaxGhenis
Copy link
Contributor

Thanks Anthony. Merging to address the immediate needs. More motivation to consider how we can improve test coverage in core though.

@MaxGhenis MaxGhenis merged commit 3bda5e9 into PolicyEngine:master Sep 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace dataset with datasets in simulation.py method
2 participants