Skip to content
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

Fix _index and HGNC symbol indexing #7

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

mossjacob
Copy link
Contributor

@mossjacob mossjacob commented Oct 7, 2024

Summary 📝

Fixes a one-line bugs in the index key extraction of an AnnData store as well as the biomart processor.

Details

  • Instead of accessing "_index" which does not exist as a key, access the _index attr which links to the key.

  • Removes the line which limits genes to those with no HGNC symbol (is there a reason this is there?)

  • Uses latest LaminDB's MappedCollection instead

@jkobject
Copy link
Owner

jkobject commented Oct 8, 2024

Hello @mossjacob,

thanks for the update! Just one question about the mapped.py update, any explanation? I take this file from lamindb's mapped function. Can you explain what it is changing?

Also, there was quite some updates recently on lamindb, if you want to update it from their version and make a PR, it would be super useful!
https://github.com/laminlabs/lamindb/blame/2e3ebeac61608f1a83f50583890db23bfec595d7/lamindb/core/_mapped_collection.py#L55

Best,

@mossjacob
Copy link
Contributor Author

Hello @jkobject,

I think it was a bug in that line--the index of var couldn't be accessed as it was. I did not realise that this file was copied from lamin--how come you don't just import MappedCollection from lamindb? I believe lamindb is a dependency of this project?

Best,
Jacob

@jkobject
Copy link
Owner

jkobject commented Oct 9, 2024

Initially I had updates that were specific to scdataloader. Now they all got added to lamin. But I have some plans to update this part soon too so I was keeping it like this.

@mossjacob
Copy link
Contributor Author

Hi,

I've updated the PR to use the latest Lamin mapped collection and added separately the check_aligned_vars function to the scDataLoader Dataset. How does it look to you?

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@jkobject jkobject merged commit 0d1e08f into jkobject:main Oct 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants