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

rationalize Index.select(...) return/exception values #1940

Closed
ctb opened this issue Apr 10, 2022 · 2 comments · Fixed by #2204
Closed

rationalize Index.select(...) return/exception values #1940

ctb opened this issue Apr 10, 2022 · 2 comments · Fixed by #2204

Comments

@ctb
Copy link
Contributor

ctb commented Apr 10, 2022

In #1936, I realized that when confronted with a select that will result in an empty Index class, some classes (LCA_Database in particular) raise a ValueError, while others just return an empty Index class.

See test_index_select_nada in particular.

I'm not sure what it should do but it would be good to have a single kind of behavior. I think I originally implemented it this way so as to provide good error messages to people when loading the wrong k-mer size for an LCA or SBT database.

@ctb
Copy link
Contributor Author

ctb commented Aug 14, 2022

I'm not sure what it should do but it would be good to have a single kind of behavior. I think I originally implemented it this way so as to provide good error messages to people when loading the wrong k-mer size for an LCA or SBT database.

I tried changing LCAs and SBTs over to returning an empty database on incompatible select in #2204, but I then reverted that change, because so much useful information was lost in the process!

Instead, once #2204 is merged, we just make an empty database in sourmash_args.load_dbs_and_sigs when select raises a ValueError. This leaves the internal API a little messier, but keeps informative error messages around.

@ctb
Copy link
Contributor Author

ctb commented Aug 14, 2022

#2204 will close this with #wontfix.

@ctb ctb closed this as completed in #2204 Aug 15, 2022
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 a pull request may close this issue.

1 participant