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

MRG: additional moltype specification needed for sig downsample with skipmers #3457

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

bluegenes
Copy link
Contributor

@bluegenes bluegenes commented Dec 27, 2024

needed to enable skipmers with sig downsample

Adding moltypes would be easier with some unified functions for handling + testing...

Copy link

codecov bot commented Dec 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.01%. Comparing base (c3791c8) to head (1dc1419).
Report is 1 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest    #3457      +/-   ##
==========================================
- Coverage   88.02%   88.01%   -0.02%     
==========================================
  Files         137      137              
  Lines       22317    22317              
  Branches     2225     2225              
==========================================
- Hits        19645    19642       -3     
- Misses       2359     2362       +3     
  Partials      313      313              
Flag Coverage Δ
hypothesis-py 25.43% <ø> (ø)
python 92.32% <ø> (ø)
rust 82.05% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bluegenes bluegenes changed the title WIP: additional changes needed for python skipmers MRG: additional moltype specification needed for sig downsample with skipmers Jan 9, 2025
@bluegenes
Copy link
Contributor Author

@ctb ready for review

@ctb
Copy link
Contributor

ctb commented Jan 9, 2025

A few comments:

  • needs update
  • CI isn't passing...
  • can you create an issue pointing at this with comment "Adding moltypes would be easier with some unified functions for handling + testing..."?

@bluegenes
Copy link
Contributor Author

CI isn't passing b/c of coverage -- which is true of the other skipmer additions as well. There are no skipmer tests in the sourmash python codebase yet. I think we decided it was ok for now b/c there is actually no way to make skipmer sigs through the python codebase, either, and these additions don't break our existing tests. But we should not call attention to skipmers in here until we add tests and docs, #3449.

The changes I've thus far added enable the following sig commands: cat, summarize, describe, downsample. It should include more (all?), but I have not run any others.

There are tests in branchwater and directsketch that cover the array of skipmer sketch building and uses there.

@ctb
Copy link
Contributor

ctb commented Jan 9, 2025

fantastic - I'd go ahead and merge :)

@bluegenes bluegenes merged commit 23b1a62 into latest Jan 9, 2025
41 checks passed
@bluegenes bluegenes deleted the more-skipmer branch January 9, 2025 19:34
@ctb ctb mentioned this pull request Jan 11, 2025
ctb added a commit that referenced this pull request Jan 11, 2025
Release issue: #3481

----

NOTE: This release adds basic support for skipmers, but they are not
yet fully supported.

Minor new features:

* add genbank plant db to docs (#3429)
* add skipmer capacity to sourmash python layer via ffi (#3446)
* add skipmers; switch to reading frame approach for translation,
skipmers (#3395)
* additional moltype specification needed for `sig downsample` with
skipmers (#3457)
* update with misc animal genomes (#3422)

Cleanup and documentation updates:

* add comment about semver and column headings (#3433)

Developer updates:

* Deps: update to rocksdb 0.23 (#3456)
* Refactor: Use to_writer/from_reader across the codebase (#3443)
* adjust `Signature::name()` to return `Option<String>` instead of
`filename()` and `md5sum()` (#3434)
* bump version to 4.8.13-dev (#3474)
* fix comment in _set_num_scaled (#3451)
* propagate zipfile errors (#3431)
* update rust CHANGELOG in preparation for r0.18.0 (#3450)
* CI: github actions updates (#3476)

Dependabot updates:

* Bump itertools from 0.13.0 to 0.14.0 (#3471)
* Bump needletail from 0.6.0 to 0.6.1 (#3427)
* Bump proptest from 1.5.0 to 1.6.0 (#3437)
* Bump roaring from 0.10.7 to 0.10.8 (#3423)
* Bump roaring from 0.10.8 to 0.10.9 (#3438)
* Bump serde from 1.0.215 to 1.0.216 (#3436)
* Bump serde from 1.0.216 to 1.0.217 (#3464)
* Bump serde_json from 1.0.133 to 1.0.134 (#3453)
* Bump statrs from 0.17.1 to 0.18.0 (#3426)
* Bump tempfile from 3.14.0 to 3.15.0 (#3472)
* Bump thiserror from 2.0.3 to 2.0.6 (#3425)
* Bump thiserror from 2.0.6 to 2.0.7 (#3435)
* Bump thiserror from 2.0.7 to 2.0.8 (#3448)
* Bump thiserror from 2.0.8 to 2.0.9 (#3452)
* Update maturin requirement from <1.8.0,>=1 to >=1,<1.9.0 (#3465)
* [pre-commit.ci] pre-commit autoupdate (#3428)
* [pre-commit.ci] pre-commit autoupdate (#3439)
* [pre-commit.ci] pre-commit autoupdate (#3454)
* [pre-commit.ci] pre-commit autoupdate (#3473)
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