set add_anndata to have an inplace argument for consistent API - #291
vbrennsteiner wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks for tackling #270!
Should we use private methods from anndata? I assume you validated that you hit all instances of add_metadata.
| metadata = metadata[~metadata.index.duplicated(keep="first")] | ||
|
|
||
| return add_metadata( | ||
| add_metadata( |
There was a problem hiding this comment.
In my opinion, in place operations in a function are a little intransparent - explicitly pass inplace=False and overwrite anndata?
| import numpy as np | ||
|
|
||
| from alphapepttools._utils import get_matrix | ||
| from alphapepttools._utils import _resolve_axis, get_matrix |
|
|
||
| If axis is 0, assume metadata.index <-> data.index and add metadata as '.obs' of the AnnData object. | ||
| If axis is 1, assume metadata.index <-> data.columns and add metadata as '.var' of the AnnData object. | ||
| If axis is "obs" or 0, assume metadata.index <-> data.index and add metadata as '.obs' of the AnnData object. |
There was a problem hiding this comment.
<-> parses weird in the documentation. Replace with corresponds to?
| adata | ||
| AnnData object to add metadata to | ||
| incoming_metadata | ||
| Metadata dataframe to add. The matching entity is always the INDEX, depending on axis it is |
There was a problem hiding this comment.
make INDEX lower case?
| If True, print additional information about the operation | ||
| inplace | ||
| If True (default), modifies adata inplace, adding metadata to the existing .obs or .var, depending on `axis`. | ||
| Note that with `keep_data_shape=False` this may also shrink `adata` in place. |
There was a problem hiding this comment.
Emit warning if anndata is filtered?
|
|
||
| ### Emulate join on AnnData level without copying | ||
| # 1. Reindex the AnnData object for inner join | ||
| # 1. Subset the AnnData object for inner join; _inplace_subset_* is `adata[index, :]`, but inplace |
There was a problem hiding this comment.
I did not understand this comment when reading from top to bottom
| adata = adata[shared_fields, :] if axis == 0 else adata[:, shared_fields] | ||
| if not existing_fields.equals(shared_fields): | ||
| if axis == "obs": | ||
| adata._inplace_subset_obs(shared_fields) # noqa: SLF001 |
There was a problem hiding this comment.
I think we should not use a private method of an external package. What do you think?
| extra_obs_df = psm_df[[sample_id_column, *extra_obs_cols]].set_index(sample_id_column, drop=True) | ||
| extra_obs_df = extra_obs_df[~extra_obs_df.index.duplicated(keep="first")] | ||
| comparison_adata = add_metadata(comparison_adata, extra_obs_df, axis=0) | ||
| add_metadata(comparison_adata, extra_obs_df, axis=0) |
There was a problem hiding this comment.
set inplace=False for internal stuff?
| extra_var_df = psm_df[[feature_id_column, *extra_var_cols]].set_index(feature_id_column, drop=True) | ||
| extra_var_df = extra_var_df[~extra_var_df.index.duplicated(keep="first")] | ||
| comparison_adata = add_metadata(comparison_adata, extra_var_df, axis=1) | ||
| add_metadata(comparison_adata, extra_var_df, axis=1) |
There was a problem hiding this comment.
Set inplace=False for internal stuff? Feels weird when a function performs an inplace operation in the pipeline
This branch has not been deployed
2 participants
Relating to #270: add
inplaceargument toadd_metadataAlso clean up logic by removing null-ops and adapting unit tests with an additional inplace decorator to check the existing cases with and without inplace modification