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

[WIP] add signatures() method to both LCA and SBT indices #796

Merged
merged 6 commits into from
Dec 15, 2019

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Dec 13, 2019

NOTE: this is a PR into #556.

Some quick notes for @luizirber -

  • the SBT class has insert already defined but it takes different arguments than Index.insert.
  • at least at the moment LCA indices don't support inserting.

tests/test_sbt.py Outdated Show resolved Hide resolved
sig47 = load_one_signature(utils.get_test_data('47.fa.sig'))
sig63 = load_one_signature(utils.get_test_data('63.fa.sig'))

tree.insert(SigLeaf('47', sig47))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the rust side it's taking a signature (sig47, in this case) and building the SigLeaf internally. (This is the code that does the conversion).

Issue: with this design you can't set metadata (I'm setting it to empty). We don't use metadata that much, but might be good to clarify what is the intended use for it (only internal properties for SBT operations? user-provided metadata?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all signature metadata should be in SourmashSignature. Maybe we can allow insert to take hints or something? Dunno. But for now is it OK if I change tree.insert to take SourmashSignature objects?

Co-Authored-By: Luiz Irber <luizirber@users.noreply.github.com>
Copy link
Member

Yup, taking a SourmashSignature is the way to go

@ctb
Copy link
Contributor Author

ctb commented Dec 14, 2019 via email

@luizirber
Copy link
Member

alas, it is not as easy as that! 'insert' is currently used to add bloom filter nodes too.
What do you think about de-deprecating add_node, and having insert construct a SigLeaf and then use add_node underneath? Then add_node would be used for bloom filter nodes.

I think that's fair. In this case we would have the Index abc implemented for the SBT with minhash leaves, but not for the 'regular' SBT (bloom filters for internal and leaf nodes).

The deprecation warning was for pointing to libraries using the SBT directly that they should think about using .insert() instead, but since add_node would continue working I don't think we need to deprecate it.

Comment on lines 214 to 216
sig = next(load_signatures(utils.get_test_data(f)))
leaf = SigLeaf(f, sig)
tree.add_node(leaf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe do

sig = load_one_signature(utils.get_test_data(f))
tree.insert(sig)

here?

@ctb ctb merged commit c6c0213 into refactor/index Dec 15, 2019
@ctb ctb deleted the refactor/index_cleanup branch December 15, 2019 05:02
@ctb ctb mentioned this pull request Dec 15, 2019
11 tasks
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.

2 participants