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

load QC models only when the tests are run #1418

Open
grahamgower opened this issue Nov 8, 2022 · 1 comment · May be fixed by #1419
Open

load QC models only when the tests are run #1418

grahamgower opened this issue Nov 8, 2022 · 1 comment · May be fixed by #1419

Comments

@grahamgower
Copy link
Member

Right now we load all QC models from stdpopsim/__init__.py, which means import stdpopsim loads all the QC models. This is (almost) unnecessary. Only the test suite makes meaningful use of the QC models---to confirm the QC model matches the main model. However, the CLI also looks at the DemographicModel.qc_model field to see if a model has been QCed and prints a warning if a user tries to simulate a model that hasn't been QCed. Right now, all our demographic models have been QCed (hooray!), so this warning doesn't apply.

Options:

  1. Keep the status quo - loading all QC models at runtime.
  2. Remove the warning about non-QCed models from the CLI.
  3. Seprate the "this model has been QCed" logic from the QC model itself. This could be done by changing the register_qc_model() method to accept a function that returns the qc model, rather than the model object itself (ie. a very small code change to remove () in lots of places). The test suite would call that function. The CLI logic just checks that the qc_model attribute is not None, so this wouldn't need to be changed at all.

Thoughts?

@jeromekelleher
Copy link
Member

Option 3 sounds great to me!

grahamgower added a commit to grahamgower/stdpopsim that referenced this issue Nov 9, 2022
@grahamgower grahamgower linked a pull request Nov 9, 2022 that will close this issue
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 a pull request may close this issue.

2 participants