-
Notifications
You must be signed in to change notification settings - Fork 346
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
Avoid clobber in setup_anndata using model instance and manager ID back references #1342
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
==========================================
- Coverage 90.81% 90.72% -0.10%
==========================================
Files 109 109
Lines 8180 8223 +43
==========================================
+ Hits 7429 7460 +31
- Misses 751 763 +12
Continue to review full report at Codecov.
|
b2641be
to
295cabb
Compare
""" | ||
|
||
def __init__(cls, name, bases, dct): | ||
cls.manager_store = dict() | ||
cls._setup_adata_manager_store: Dict[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if I'm wrong, but this manager store will only ever receive one anndata now, the one used to initialize the model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is the case, should we make it not a dict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this is general enough...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it stores it for the whole class. this allows for situations where an AnnData is setup once and used to create two separate models.
…ck references (scverse#1342) * add failing test * avoid clobber using back reference to manager and model uuids * fix arches mixin * fix gimvi tests * add additional test, address minor comments * make more explicit manager stores for each case * address second round comments
This PR introduces model instance and AnnDataManager ids (uuid4) to avoid clobbering between models referencing the same AnnData object with different parameters.
This is accomplished by adding a model instance ID which is used to create a model instance specific mapping to AnnDataManagers used to initialize given models. The same goes with AnnDataManagers created via
transfer_setup
on different AnnData objects (e.g. viaget_latent_representation()
). This allows model instances to reliably fetch AnnDataManagers created for/by that model instance, rather than the last AnnDataManager created for this AnnData object in this class.Secondly, it is still possible for an AnnData object to be mutated by another AnnDataManager's
register_fields()
function call. This can result in a situation where a model instance retrieves the correct AnnDataManager object, but the underlying AnnData has changed. To avoid this situation, we replacesource_scvi_uuid
withscvi_last_manager_uuid
which reports which AnnDataManager instance was used last to setup the AnnData object. Now, one can calladata_manager.validate()
to check whether the AnnData has been tampered with, and if so rerunregister_fields()
with the original arguments to recreate the desired setup.