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: core: add scaled selection to manifest; add helper functions for collection and sig/sketch usage #2948

Merged
merged 19 commits into from
Feb 5, 2024

Conversation

bluegenes
Copy link
Contributor

@bluegenes bluegenes commented Jan 27, 2024

This PR adds:

New functions:

  • Collection::sig_from_record

when we iterate through the sketches, we have both idx and record available. I thought it would make sense to just use record directly, rather than re-getting record from index.

  • Signature::minhash

if there is one minhash sketch available within the sig that matches selection params, return it

  • Signature::get_sketch

if there is one sketch (of any type) available within the sig that matches selection params, return it. Note that since this returns the sketch enum, it still requires checking MinHash type afterwards. @luizirber is there a way to return any of the sketches directly from the same function (like minhash function, above, but more flexible?).

  • Manifest::From<&PathBuf>

build a manifest directly from a pathlist file. Added and tested, but lmk if you think we should just build a list of paths separately. I wanted this for branchwater, but am not actually using it since neither the paths or PathBuf loading code allow missing/failed paths.

New selection functionality

  • added scaled and num selection to manifest. For scaled, if sketch is compatible (equal scaled or can be downsampled), keep it during manifest selection. Otherwise, discard.

Tests added for each new function/added code.


Context and prior comments

There are a few things I found difficult to use in sourmash-bio/sourmash_plugin_branchwater#197. This PR is to propose some changes that might simplify things.

In the new core, when we want minhash from a signature (SigStore), we need to go through a couple checks because it's in a sketches vec within in a Sketch enum, However, when we're loading from a collection, like with collection.sig_for_dataset, we know we only have a single sketch (AND we know it's a MinHash type, because we ran Select on it with scaled). Indeed, probably this is the case often, because sig select is doing the work?

from luiz:
ok, let's see:

  • the assert_eq! is there because after Select there should be only one matching sketch inside the sig. The selection here is initialized from a Record (a line) in the Manifest, which might have one sig file with different params (common one: k=21,31,51, leading to three lines in the manifest), but since sig_for_dataset takes a dataset_id (which refers to one Record/line), it should always return one sketch in one sig.
  • we can have a .minhash() method in Signature as a shortcut, but will have similar logic to load_single_sketch_from_sig. It can be much shorter:
    pub fn minhash(&self) -> Option<&KmerMinHash> {
        for sk in &self.signatures {
            // TODO:  also account for Sketch::LargeMinHash?
            if let Sketch::MinHash(mh) = sk {
                // TODO: might want to iterate over all and guarantee only
                // one minhash was around
                return Some(mh);
            }
        }
        None
    }

Other notes from Luiz to address or file in an issue:

  • .sketches() is absolutely terrible and we should find a replacement. It clones all the data in the Signature, which can be a LOT of useless work
  • rust and python code should support same ops. Luiz started unifying a bunch of the code in Feature: Mutable/Frozen MH in FFI #2728 but at this point that's very outdated

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fee6292) 86.60% compared to head (624c7d7) 86.65%.

Files Patch % Lines
src/core/src/signature.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           latest    #2948      +/-   ##
==========================================
+ Coverage   86.60%   86.65%   +0.04%     
==========================================
  Files         135      135              
  Lines       15264    15284      +20     
  Branches     2592     2592              
==========================================
+ Hits        13220    13244      +24     
+ Misses       1744     1740       -4     
  Partials      300      300              
Flag Coverage Δ
hypothesis-py 25.80% <ø> (ø)
python 92.82% <ø> (ø)
rust 59.52% <91.30%> (+0.42%) ⬆️

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: changes to facilitate new core sig and sketch use MRG: core: add scaled selection to manifest; add helper functions for collection and sig/sketch usage Jan 31, 2024
@bluegenes
Copy link
Contributor Author

ok @ctb @luizirber ready for review!

fyi - looks like they python tests are passing but then failing to upload the report to codecov. I'll rerun the tests in a bit.

@ctb
Copy link
Contributor

ctb commented Jan 31, 2024

fyi - looks like they python tests are passing but then failing to upload the report to codecov. I'll rerun the tests in a bit.

that does happen a lot... our codecov stuff is routinely the most brittle part of our CI.

@ctb
Copy link
Contributor

ctb commented Jan 31, 2024

Tests now pass!

src/core/src/signature.rs Outdated Show resolved Hide resolved
src/core/src/signature.rs Outdated Show resolved Hide resolved
Copy link
Member

@luizirber luizirber left a comment

Choose a reason for hiding this comment

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

Other than the replacement of dbg! with todo!, this looks great! and so many tests! =]

@bluegenes bluegenes merged commit 9f36e2f into latest Feb 5, 2024
39 checks passed
@bluegenes bluegenes deleted the core-upds-for-branchwater-plugin branch February 5, 2024 18:57
ctb added a commit that referenced this pull request Feb 10, 2024
MSRV: 1.65

Changes/additions:

* Clean up and refactor `KmerMinHash::merge` in core (#2973)
* core: add scaled selection to manifest; add helper functions for
collection and sig/sketch usage (#2948)
* core: enable downsample within select (#2931)

Updates:

* Deps: update typed-builder and histogram, bump MSRV to 1.65 (#2858)
* Bump tempfile from 3.9.0 to 3.10.0 (#2979)
* Bump rkyv from 0.7.43 to 0.7.44 (#2978)
* Bump memmap2 from 0.9.3 to 0.9.4 (#2958)
* Bump chrono from 0.4.31 to 0.4.33 (#2957)
* Bump serde from 1.0.195 to 1.0.196 (#2956)
* Bump serde_json from 1.0.111 to 1.0.113 (#2955)
* Bump web-sys from 0.3.66 to 0.3.67 (#2939)
* Bump wasm-bindgen-test from 0.3.39 to 0.3.40 (#2938)
* Bump rayon from 1.8.0 to 1.8.1 (#2937)
* Bump ouroboros from 0.18.2 to 0.18.3 (#2936)
* Bump histogram from 0.8.4 to 0.9.0 (#2935)
* Bump wasm-bindgen from 0.2.89 to 0.2.90 (#2925)
* Bump histogram from 0.8.3 to 0.8.4 (#2923)
* Bump serde_json from 1.0.110 to 1.0.111 (#2902)
* Bump serde from 1.0.194 to 1.0.195 (#2901)
* Bump serde_json from 1.0.108 to 1.0.110 (#2896)
* Bump ouroboros from 0.18.1 to 0.18.2 (#2894)
* Bump tempfile from 3.8.1 to 3.9.0 (#2893)
* Bump memmap2 from 0.9.2 to 0.9.3 (#2889)
* Bump memmap2 from 0.9.0 to 0.9.2 (#2882)
* Bump rkyv from 0.7.42 to 0.7.43 (#2880)
* Bump ouroboros from 0.18.0 to 0.18.1 (#2875)
* Bump once_cell from 1.18.0 to 1.19.0 (#2874)
* Bump rkyv from 0.7.40 to 0.7.42 (#2863)
* Bump csv from 1.2.0 to 1.3.0 (#2862)
* Bump roaring from 0.10.1 to 0.10.2 (#2865)
* Bump web-sys from 0.3.65 to 0.3.66 (#2864)
* Bump byteorder from 1.4.3 to 1.5.0 (#2866)
* Bump proptest from 1.3.1 to 1.4.0 (#2837)
@ctb ctb mentioned this pull request Feb 10, 2024
ctb added a commit that referenced this pull request Feb 10, 2024
release notes: https://hackmd.io/SCoVcWS1RhCH-ndQNWF-1A?view

# sourmash release v4.8.6 - release notes

Minor new features:

* re-establish `tax` gather reading flexibility (#2986)
* update JOSS paper per pyopensci review (#2964)
* Clean up and refactor `KmerMinHash::merge` in core (#2973)
* add label output & input options to `compare` and `plot`, for better
customization (#2598)
* add utilities for using ictv taxonomic ranks with `sourmash tax`
(#2608)

Bug fixes:

* Fix `tax metagenome` to work on gather output created with
`--estimate-ani-ci` (#2952)
* fix gather memory usage issue by not accumulating `GatherResult`
(#2962)
* update the CLI docs and help for `search --containment` and `prefetch`
(#2971)

Documentation updates:

* update tutorial to remove bioconda & use sourmash-minimal (#2972)
* update readme with maintainers & sourmash comparison info (#2965)
* add branchwater reference; make FAQ more visible (#2984)
* update FAQ answer on k-mer size (#2899)
* update README with repostatus and pyver badges, and Windows support
(#2928)
* add full column descriptions for `gather` and `prefetch` output
(#2954)
* add scaled FAQ, adjust ksize answer (#2921)
* minor refactoring of gather code, small doc updates (#2953)
* Add threshold-bp and scaled relationship to faqs (#2930)

Developer updates:

* nix updates for pyopensci review (#2975)
* add scaled selection to manifest; add helper functions for collection
and sig/sketch usage (#2948)
* Pre-commit updates (#2427)
* fix upload wheel CI (#2974)
* release core; bump rust core version to r0.12.1 (#2988)
* CI: macos deployment target and maturin updates (#2879)
* MRG: bump version to 4.8.6-dev, post-release (#2877)
* fix benchmark code & a few other small issues from pyOpenSci review
(#2920)
* fix uploading of wheels after upload-artifact upgrade. (#2887)
* in core, enable downsample within select (#2931)

Dependabot updates:

* Bump pypa/cibuildwheel from 2.16.4 to 2.16.5 (#2981)
* Bump tempfile from 3.9.0 to 3.10.0 (#2979)
* Bump rkyv from 0.7.43 to 0.7.44 (#2978)
* Bump actions/cache from 3 to 4 (#2933)
* Bump actions/download-artifact from 3 to 4 (#2884)
* Bump actions/upload-artifact from 3 to 4 (#2883)
* Bump cachix/cachix-action from 13 to 14 (#2926)
* Bump cachix/install-nix-action from 24 to 25 (#2927)
* Bump chrono from 0.4.31 to 0.4.33 (#2957)
* Bump getrandom from 0.2.11 to 0.2.12 (#2924)
* Bump histogram from 0.8.3 to 0.8.4 (#2923)
* Bump histogram from 0.8.4 to 0.9.0 (#2935)
* Bump jinja2 from 3.1.2 to 3.1.3 (#2922)
* Bump memmap2 from 0.9.0 to 0.9.2 (#2882)
* Bump memmap2 from 0.9.2 to 0.9.3 (#2889)
* Bump memmap2 from 0.9.3 to 0.9.4 (#2958)
* Bump mymindstorm/setup-emsdk from 13 to 14 (#2934)
* Bump ouroboros from 0.18.1 to 0.18.2 (#2894)
* Bump ouroboros from 0.18.2 to 0.18.3 (#2936)
* Bump pypa/cibuildwheel from 2.16.2 to 2.16.4 (#2960)
* Bump rayon from 1.8.0 to 1.8.1 (#2937)
* Bump rkyv from 0.7.42 to 0.7.43 (#2880)
* Bump serde from 1.0.194 to 1.0.195 (#2901)
* Bump serde from 1.0.195 to 1.0.196 (#2956)
* Bump serde_json from 1.0.108 to 1.0.110 (#2896)
* Bump serde_json from 1.0.110 to 1.0.111 (#2902)
* Bump serde_json from 1.0.111 to 1.0.113 (#2955)
* Bump shlex from 1.1.0 to 1.3.0 (#2940)
* Bump supercharge/redis-github-action from 1.7.0 to 1.8.0 (#2885)
* Bump tempfile from 3.8.1 to 3.9.0 (#2893)
* Bump thiserror from 1.0.50 to 1.0.51 (#2881)
* Bump thiserror from 1.0.51 to 1.0.56 (#2897)
* Bump wasm-bindgen from 0.2.89 to 0.2.90 (#2925)
* Bump wasm-bindgen-test from 0.3.39 to 0.3.40 (#2938)
* Bump web-sys from 0.3.66 to 0.3.67 (#2939)
* Update pytest requirement from <7.5.0,>=6.2.4 to >=6.2.4,<8.1.0
(#2959)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants