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

Only load QC models when running tests #1419

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

grahamgower
Copy link
Member

Closes #1418

@jeromekelleher
Copy link
Member

Looks good. Can we also remove the from . import qc in __init__?

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Base: 99.94% // Head: 99.94% // No change to project coverage 👍

Coverage data is based on head (080a5f8) compared to base (b1383cc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1419   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files         109      109           
  Lines        3807     3807           
  Branches      529      529           
=======================================
  Hits         3805     3805           
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
stdpopsim/qc/AnaPla.py 100.00% <ø> (ø)
stdpopsim/dfe.py 100.00% <100.00%> (ø)
stdpopsim/models.py 99.07% <100.00%> (ø)
stdpopsim/qc/AnoGam.py 100.00% <100.00%> (ø)
stdpopsim/qc/AraTha.py 100.00% <100.00%> (ø)
stdpopsim/qc/BosTau.py 100.00% <100.00%> (ø)
stdpopsim/qc/DroMel.py 100.00% <100.00%> (ø)
stdpopsim/qc/HomSap.py 100.00% <100.00%> (ø)
stdpopsim/qc/PanTro.py 100.00% <100.00%> (ø)
stdpopsim/qc/PapAnu.py 100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@grahamgower
Copy link
Member Author

Looks good. Can we also remove the from . import qc in __init__?

That would have been the simpler solution, but the CLI checks if qc_model is None to decide whether to print a warning. So we need to flag models as having a qc model available somehow.

qc_complete = model.qc_model is not None

stdpopsim/stdpopsim/cli.py

Lines 837 to 850 in b1383cc

if not qc_complete:
warnings.warn(
stdpopsim.QCMissingWarning(
f"{model.id} has not been QCed. Use at your own risk! "
"Demographic models that have not undergone stdpopsim's "
"Quality Control procedure may contain implementation "
"errors, leading to differences between simulations "
"and the model described in the original publication. "
"More information about the QC process can be found in "
"the developer documentation. "
"https://popsim-consortium.github.io/stdpopsim-docs/"
"latest/development.html#demographic-model-review-process"
)
)

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so this prevents the run-time evaluation of the QC model but not the importing of the code. So, we couldn't do any extra imports in the qc model (unless we did icky things like put them in the function).

What if we moved the qc package into the tests directory and imported that at run-time? It could add on the qc_model attribute which we'd use for these tests (or we could come up with a less icky way of doing it later).

We could even just do something like removing the import from stdpopsim/__init__.py and explicitly import stdpopsim.qc from tests/test_models.py. I think that would do the trick actually.

Then, we replace the current check to see if there is QC model with a static is_qc_checked attribute on the class, which we set to True for all existing models and we document the process of setting this to True as part of the QC process?

@grahamgower
Copy link
Member Author

Yeah, I agree that the only way to avoid importing the qc module is to change the QC process to manually do something to the model as part of the QC (e.g. setting a model flag as you suggest). Maybe we defer this until after the conversion of models to Demes files though? The model implementation and QC process docs will need modifying then anyhow, and we can probably flag models as having been QCed in other ways then (e.g. put non-QCed model files in "staging" folder or something).

@jeromekelleher
Copy link
Member

Yeah, sounds good. I just wonder if there's much point in making this change now if it's not really going to affect load time or allow us to include any extra packages for QC.

@grahamgower grahamgower marked this pull request as draft November 9, 2022 15:55
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.

load QC models only when the tests are run
2 participants