Repository navigation
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
==========================================
- Coverage 42.49% 41.33% -1.17%
==========================================
Files 20 20
Lines 2826 3019 +193
==========================================
+ Hits 1201 1248 +47
- Misses 1625 1771 +146
🚀 New features to boost your workflow:
|
VladimirShitov
left a comment
VladimirShitov
left a comment
There was a problem hiding this comment.
Overall, I think it's great idea to bring a specialised data structure to back up patpy. Print output of DonorData objects look great and I like the MIL loader. However, this PR needs restructuring and better use cases. Now it looks like a lot of extra code with unclear benefits. Here ar esome suggestions (besides inline comments):
- Can you add a small code example (or a tutorial) on using DonorData? How to use it, what is stored where and why is it more convenient than separate objects?
- Is donor / sample division necessary? It brings quite some extra engineering, but I don't see where it helps. In fact, it can make things worse, e.g. by biasing evaluation metrics with repeated samples. So far, in patpy, I followed approach "everything is a sample" and let the users define what it actually is – e.g. donor. I would suggest following the same approach and removing additional level of complexity on everythign regarding "units" in code.
| return adata | ||
|
|
||
|
|
||
| def as_donordata(adata: AnnData, info: DatasetInfo, meta_adata: AnnData | None = None, keep_in_cells: bool = True): |
There was a problem hiding this comment.
This is not the right place for it: datasets module contains a collection of datasets, not the other functionality. Feel free to create io module or put this into pp
| Parameters | ||
| ---------- | ||
| adata | ||
| Cells of a dataset loaded with patpy. |
There was a problem hiding this comment.
| Cells of a dataset loaded with patpy. | |
| Annotated data object containing cells |
| adata | ||
| Cells of a dataset loaded with patpy. | ||
| info | ||
| The ``DatasetInfo`` of the dataset, whose ``sample_key`` names the samples and whose |
There was a problem hiding this comment.
Chain of dependencies becomes a bit long - a user must create dataset info, then donordata. Let's just follow DatasetInfo interface here. Make sample_key and sample_metadata_columns arguments of this function. Users can then easily provide them from DatasetInfo or just enter manually
|
|
||
| Returns | ||
| ------- | ||
| donordata.DonorData |
There was a problem hiding this comment.
A description of what exactly it is (e.g. how to access cells and donors) would be nice
| Examples | ||
| -------- | ||
| >>> adata, info = patpy.datasets.stephenson(return_dataset_info=True) | ||
| >>> dd = patpy.datasets.as_donordata(adata, info) |
There was a problem hiding this comment.
Let's go with ddata at least. I don't like variables that are difficult to understand without context
| known = {c for _, table in self._unit_tables() for c in table.columns} | ||
| return [c for c in columns if c in known and c != self.sample_key] | ||
|
|
||
| def _unit_table(self, columns=None) -> pd.DataFrame: |
| return pd.Series(adata[:, col].X.toarray().flatten(), index=adata.obs_names) | ||
|
|
||
|
|
||
| def _representation_distances(meta_adata, representation: str): |
There was a problem hiding this comment.
I do not think this function is needed and it's only used once
| """Return *meta_adata*, or the samples of a DonorData as units when its representations live there. | ||
|
|
||
| A DonorData with a sample level keeps sample representations in ``obsm`` and ``obsp`` of its | ||
| samples. :meth:`~donordata.DonorData.flatten` turns the samples into the units, with the donor |
There was a problem hiding this comment.
Hmm, I'm wondering if this will lead to problems in some metrics by repeating the same information several times
There was a problem hiding this comment.
In any case, _evaluation module is the wrong place for that. In general, because it's very DonorData-specific, I would suggest putting it to DonorData package
| trainer.fit(self._pascient_model, train_dataloaders=train_dl, val_dataloaders=val_dl) | ||
| self._pascient_model.eval() | ||
|
|
||
| def _training_loaders(self, adata: sc.AnnData, label_key: str, y_map: dict, task: str, batch_cls): |
| "sphinxext-opengraph", | ||
| ] | ||
| optional-dependencies.donordata = [ | ||
| "donordata @ git+https://github.com/theislab/donordata.git", |
There was a problem hiding this comment.
This will unfortunately not make it to the pypi package. Git links are not allowed. Please do either:
- Upload your package to pypi and add pypi dependency
- Remove dependency from here and add a suggestion to install it on iomort, e.g.:
try:
import donordata
except ImportError:
print("Import error. Please install donordata: `git+https://github.com/theislab/donordata.git@main`")
There was a problem hiding this comment.
I pinged @LArnoldt for the release, which he was very thankful for
| rep = pd.DataFrame(rep, index=self.samples) | ||
| return rep | ||
|
|
||
| def to_donordata(self, key: str | None = None, donor_data=None): |
There was a problem hiding this comment.
I gotta double-check Lucas' latest activities on donordata updates.
The rough functionality here could be shouldered on cellink/donordata site I think though, if a major issue for Vlad is to avoid code overhead.
No description provided.