Allow xarray Datasets to be used for obs/var/obsm/varm #1966
Conversation
ilia-kats
force-pushed
the
xarray_dataset
branch
from
0083532 to
f297a74
Compare
it's being used in many places in _core already
ilia-kats
force-pushed
the
xarray_dataset
branch
from
46efe51 to
e647d94
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
ilia-kats
force-pushed
the
xarray_dataset
branch
from
db95d9b to
80563bb
Compare
for more information, see https://pre-commit.ci
it can now handle the same dependency being specified multiple times with different min. versions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1966 +/- ##
==========================================
+ Coverage 83.34% 85.09% +1.75%
==========================================
Files 47 47
Lines 6856 6959 +103
==========================================
+ Hits 5714 5922 +208
+ Misses 1142 1037 -105
🚀 New features to boost your workflow:
|
|
I'm not sure what to do about the documentation warnings. I think the problem is that it's building the documentation for |
to be re-added when scverse/anndata#1966 is merged
(fix): small fix for new xarray + test deps
(chore): add `obsm` access test
|
@ilia-kats Is there anything else we are missing here? I will give this a final look tomorrow. I'm hoping to bundle this, the upcoming zarr release as a minimum bound, and the upcoming xarray release as another minimum bound, into one new release candidate. |
|
If your xarray PR is merged before the next release I would remove the workaround, and I would appreciate some advice on what to do about the docs (see this comment). That's it, I think. |
Opened a PR on this |
(fix): docs
(fix): pandas min
Co-authored-by: Ilan Gold <[email protected]>
| experimental.backed._lazy_arrays.MaskedArray | ||
| experimental.backed._lazy_arrays.CategoricalArray | ||
| experimental.backed._xarray.Dataset2D | ||
| _core.xarray.Dataset2D |
There was a problem hiding this comment.
No public exports should start with an underscore, this should be exported somewhere else.
We talked about
anndata.typesfor types that are not ABCs but intended to be used inisinstancechecksanndata.typingfor types that are only for annotations
There was a problem hiding this comment.
I already had this comment as a pending review but never submitted it :(
There was a problem hiding this comment.
Yes, apologies you are 100% right.
| min_version = Version(spec.version) | ||
|
|
||
| return Requirement(f"{req_name}=={min_version}.*") | ||
| return Requirement(f"{req_name}~={min_version}.0") |
There was a problem hiding this comment.
The == was intentional. If a patch version has issues, the minimum version should be bumped to a version that makes the tests pass.
Not everything uses semver, especially not Python land.
We can discuss this of course, if there’s a good reason to make this change we can make it, I’m just saying that this needs to be motivated.
3 participants
As discussed. This PR also moves the
Dataset2Dclass from experimental into core. IMHO this makes more sense, since it as already used all over core and is now used even more.Note that this is not a full-featured replacement for DataFrames: Pandas Extension Arrays are lost durint concatenation, since they are not supported by Dask Arrays. This affects categoricals and nullables. I would thus still like to have Dask DataFrame support.