-
Notifications
You must be signed in to change notification settings - Fork 94
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
Clarification about ID type in the docs for Table.update_ids #879
Conversation
Also fixed a grammar issue (lets --> let's).
It looks like the build is failing for this -- since this PR only contains docs fixes, these failures are likely due to dependencies shifting. The build seems to be failing when attempting to install
... I'm not sure how to fix this! It seems like |
That is unusual, would it be possible to add "anndata" to the conda install in gh actions? It's present in the bioconda channel. I wonder if pip and conda are fighting |
@wasade awesome, installing anndata via conda instead of via pip fixed the build! For now, the CI installs anndata from conda-forge instead of bioconda -- anndata is available on both of these channels (conda-forge, bioconda), but it looks like only the conda-forge version is actively maintained. I also went ahead and updated the README a bit, similar to the changes made for the iow PR. |
Confirming |
Thanks for the bump on this. Is this PR good to go? I was out all summer on leave and missed the follow up here |
The PR should be good to go. Thanks! |
Thanks! |
Just as a heads up, it looks like the Python 3.6 build began failing in recent months (although the Python 3.10 build is fine). As far as I can tell, this is due to anndata 0.8.0 bumping its minimum Python version to 3.7 (source)—biom doesn't set a pin on the anndata version used. I guess the two obvious ways to handle this issue would be 1) dropping biom's support for Python 3.6 also, or 2) pinning its anndata dependency to be |
I don't know the implications of pinning anndata. Would that be bad? |
It seems to be an explicitly optional dependency that's just used in the Looking through the code some more, a better fix might just be removing the Ultimately not my call, though :) |
Python 3.6 is EOL, so at some point you'll be dropping it anyway: https://peps.python.org/pep-0494/ |
We can account for that on the next release, but I do not have a timeline at the moment for that |
I'm not sure if non-string IDs were ever officially supported for BIOM tables, but some of the Empress test code (these lines, in particular) involved setting IDs to integers. It looks like this commit broke this behavior, and by extension this particular Empress test -- it triggered the following error:
This PR documents explicitly that both keys and values for the
id_map
parameter ofTable.update_ids()
should be strings. I haven't added any validation code that checks this (I'm not sure if the added runtime incurred by such a check would be desirable, and this seems like it'd be a pretty weird thing for users to try to do normally...?), but hopefully this clarifies things for future devs who encounter this problem on updating to newer versions ofbiom-format
.This PR also includes two small grammar fixes.
Thanks!