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

make load_one_signature use load_file_as_signatures API #1062

Closed
ctb opened this issue Jun 29, 2020 · 5 comments · Fixed by #3161
Closed

make load_one_signature use load_file_as_signatures API #1062

ctb opened this issue Jun 29, 2020 · 5 comments · Fixed by #3161

Comments

@ctb
Copy link
Contributor

ctb commented Jun 29, 2020

right now it's based on signature.load_signatures which is less flexible than sourmash_args.load_file_as_signatures.

also examine load_query_signature and see if there's duplication.

@ctb ctb changed the title make load_single_signature use load_file_as_signatures API make load_one_signature use load_file_as_signatures API Jan 23, 2021
@ctb
Copy link
Contributor Author

ctb commented Jan 23, 2021

ok, dug into this a little bit when working on #1279.

  • load_one_signature is used in a variety of places for loading from either a buffer, or a string, or a file. It uses iterators internally, so will not load all the signatures in a file.
  • load_query_signature is used in only a few places, and is only used for CLI functionality around loading a query signature from a file. It is not very well written.

My current hot take is that

I'm not sure how to rename them tho.

@ctb ctb changed the title make load_one_signature use load_file_as_signatures API make load_one_signature use load_file_as_signatures API Jun 24, 2021
@ctb
Copy link
Contributor Author

ctb commented May 13, 2024

Huh. Working on this over in #3153, and I noticed that signature.load_one_signature is used widely in the tests, but not many other places - SBT code and sig subcommand code.

@ctb
Copy link
Contributor Author

ctb commented May 13, 2024

aaaand load_signatures is hardly used anywhere in the code base, either:

sourmash/index/__init__.py
sourmash/save_load.py
sourmash/signature.py

so we've done a great job of removing this from the core sourmash code.

(Both functions in signature.py are used all over the place in the tests, tho.)

@ctb
Copy link
Contributor Author

ctb commented May 14, 2024

This is being cleaned up a lot in #3161 which will -

  • rename sourmash.signature.load_signatures to load_signatures_from_json;
  • rename sourmash.signature.load_one_signature to load_one_signature_from_json;
  • rename sourmash.signature.save_signatures to save_signatures_to_json;

@ctb
Copy link
Contributor Author

ctb commented May 14, 2024

This will be fixed when #3153 and #3161 are merged -

ctb added a commit that referenced this issue Jun 4, 2024
…signatures (#3153)

Fix `sig overlap` and `sig subtract` to take more than just JSON
signatures.

Also, adds a function `sourmash_args.load_one_signature` that I think
should (eventually) replace the now-deprecated
`sourmash.signature.load_one_signature`. This will be the topic of a new
PR - for now, I think it's a nice quick fix!

Fixes #3136

Related issues:
* #1062 - will do another
PR to close this issue
* #1877
* #1312
* #1060

TODO:
- [x] test uncovered code
- [x] do a bit more of a search and digest of related issues to see if
there's other low hanging fruit

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@ctb ctb closed this as completed in #3161 Jun 4, 2024
ctb added a commit that referenced this issue Jun 4, 2024
Note: PR into #3153

Tackles some signature loading and saving cleanup throughout the
codebase. Most changes are in the tests, and this is a significant
cleanup of the test code!

Fixes #1062.

---

Goals:
* deprecate external use of `sourmash.signature` load/save functions,
because they are JSON-specific and inflexible.
* simplify and standardize signature load/save function usage during
tests;
* get rid of deprecation messages during tests;

In brief,

* rename `sourmash.signature.load_signatures` to
`load_signatures_from_json`;
* rename `sourmash.signature.load_one_signature` to
`load_one_signature_from_json`;
* rename `sourmash.signature.save_signatures` to
`save_signatures_to_json`;
* deprecate `sourmash.save_signatures` and `sourmash.load_one_signature`
for 5.0 (joining `load_signatures`, which was already deprecated);
* reduce/eliminate deprecations by transitioning internal test code to
use these three functions directly from `sourmash.signature` instead of
from the top-level sourmash import.
* **bonus**: eliminate zipfile UserWarning around overwriting files,
which causes lots of warnings when running tests.

---

Done:
- [x] in sourmash.signature submodule, rename `load_signatures` to
`load_signatures_from_json`, `load_one_signature` to
`load_one_signatures_from_json`, and `save_signatures` to
`save_signatures_to_json`; make tests pass.
- [x] deprecate `sourmash.load_one_signature` and
`sourmash.save_signatures`.
- [x] catch zipfile UserWarning for duplicate filenames in
ZipStorage.save

TODO:
- [x] transition internal sourmash code+tests away from deprecated
functions
- [ ] create issue around changing API documentation prior to 5.0;

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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