-
Notifications
You must be signed in to change notification settings - Fork 603
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
(feat): raising errors where backed
is not supported
#3048
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3048 +/- ##
==========================================
+ Coverage 75.80% 75.87% +0.06%
==========================================
Files 110 110
Lines 12502 12533 +31
==========================================
+ Hits 9477 9509 +32
+ Misses 3025 3024 -1
|
@flying-sheep What do you think here? If the plan looks good for this subset of functions, I'd expand it. |
I like the idea! Better error messages, and getting our modalities a bit under control is a great goal as well! |
… into ig/backed_not_implemented
Looking at this again, now that I have gone through everything, I think we actually need to check types directly and shouldn't rely on |
If that’s actually supported, we need to rethink |
If what is actually supported? |
… into ig/backed_not_implemented
f25bddc
to
27c3d08
Compare
|
So it looks like we definitely started downloading the rc for numpy relecently: https://dev.azure.com/scverse/scanpy/_build/results?buildId=6661&view=logs&j=cb4d9293-b492-5d67-02b0-e6a595893958&t=22c10d56-3e3b-5f98-5bc6-b33384a21306 (from last week or something, downloading 1.26.4) vs https://dev.azure.com/scverse/scanpy/_build/results?buildId=6692&view=logs&j=cb4d9293-b492-5d67-02b0-e6a595893958&t=efb91c47-e839-5730-ecc5-cc752bc791b5 (downloading the 2.0 rc) |
xfail_if_dev_tests = pytest.mark.xfail(
os.environ.get("DEPENDENCIES_VERSION", "latest") == "pre-release",
reason="...",
)
@xfail_if_dev_tests
def test_xzy(): ... You probably need to change the tests so it makes the CI variable visible as an env variable, I’m not an Azure expert so I don’t know if it already is.
Yeah, maybe, let’s see once everything passes. I’m also OK with lowering the percentage, I just set it to 75% to have some indication if codecov is broken or working. (Before it would report 20% for a PR and there would be no visual indication that that’s a problem) |
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.
See above
Flaky tests, it seems, Will try re-running |
…rted (#3072) Co-authored-by: Ilan Gold <[email protected]> Co-authored-by: Philipp A <[email protected]>
The idea here is to raise errors where I have checked that things currently don't work, regardless of the reason why, and do not make any attempt to fix this problem. Once scverse/anndata#1469 is merged, we can make concrete recommendations for how to handle out-of-core data.
I think a decorator could work but we would have to check the type in the decorator like (instead of relying on current checks like in
filter_genes
):But then there is something like
log1p
where we quasi-supportbacked
via thischunked
kwarg, which would no really fit the above paradigm.Nonetheless, I think I need to go one-by-one through the functions to check what we support and don't.
Separately, we may want to drop support where it exists already (which from my searching, is only
obs_df
andvar_df
and thensubsample_counts
).