MRG: fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` #2722

ctb · 2023-08-19T15:31:01Z

This PR:

adds documentation for multigather to sourmash docs!
builds on MRG: Uniquify csv output from multigather #2065 / MRG: update #2065 (uniquify CSV output from multigather) #2721 so that tests pass.
adds an option -U/--output-add-query-md5sum to sourmash multigather
adds an option --force-allow-overwrite-output to sourmash multigather
CHANGES BEHAVIOR of multigather by treating query.filename == '-' as if query.filename is empty, thus replacing it with md5sum
CHANGES BEHAVIOR of multigather by failing loudly and clearly if output files are going to be overwritten
adds -E/--extension to allow output to files other than .sig

See discussion over in #2328: multigather CSV output uses signature filename as basename.

To add:

tests for -U;
implement and test -E/--extension
implement and test --force-allow-overwrite-output
fix for query_filename being None/empty in -U branch
documentation update for changed output behavior for multigather: '-' => using md5sum
documentation update for changed output behavior for multigather: fails if overwrite happens
fix multigather link in docs

Hello! Hope you and yours are doing well. I am using multigather to query protein sequences against each other, and primarily use the csv file for downstream processing. As is, if the signatures in `--query query.sig` were created from one fasta file with e.g. `--singleton`, then all results iteratively overwrite each other into the same csv file. This proposed change adds the md5sum to the query file to ensure uniqueness. Other suggestions are welcome! (not tested yet) Warmest, Olga

…o ctb_update_2065

codecov · 2023-08-19T15:34:41Z

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.78%. Comparing base (0d2359d) to head (1265d82).

Files	Patch %	Lines
src/sourmash/commands.py	87.50%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #2722      +/-   ##
==========================================
+ Coverage   86.76%   86.78%   +0.01%     
==========================================
  Files         136      136              
  Lines       15509    15523      +14     
  Branches     2624     2626       +2     
==========================================
+ Hits        13457    13471      +14     
  Misses       1751     1751              
  Partials      301      301

Flag	Coverage Δ
hypothesis-py	`25.53% <0.00%> (-0.03%)`	⬇️
python	`92.86% <88.57%> (+<0.01%)`	⬆️
rust	`61.20% <ø> (ø)`

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.

Note: PR into #2065. This PR updates #2065 with all the changes from `latest`. This includes the fix & update to multigather in #2322, which: - fixes the output of multigather to include more than one line 😆 - prints out the output filename - supports `--output-dir` No functional changes are made beyond the merge, so some tests are still failing; will discuss fixes in yet a new PR :). --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Keya Barve <53328492+keyabarve@users.noreply.github.com> Co-authored-by: ccbaumler <63077899+ccbaumler@users.noreply.github.com> Co-authored-by: Tessa Pierce Ward <bluegenes@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Reiter <taylorreiter@gmail.com> Co-authored-by: Erik Young <eeyoung@ucdavis.edu> Co-authored-by: David Koslicki <dmkoslicki@gmail.com> Co-authored-by: Luiz Irber <luizirber@users.noreply.github.com> Co-authored-by: Colton Baumler <baumlerc@farm.ucdavis.edu> Co-authored-by: Luiz Irber <contact+github@luizirber.org> Co-authored-by: N. Tessa Pierce-Ward <ntpierce@gmail.com> Co-authored-by: Peter Cock <p.j.a.cock@googlemail.com> Co-authored-by: Francesco Beghini <francesco.beghini@yale.edu> Co-authored-by: Jason Stajich <jason.stajich@ucr.edu> Co-authored-by: Katrin Leinweber <9948149+katrinleinweber@users.noreply.github.com>

…o olgabot-patch-2

…rmash into fix_multigather_output

…o fix_multigather_output

…o fix_multigather_output_merge

for more information, see https://pre-commit.ci

ctb · 2024-02-29T15:23:20Z

OK, I'm proposing this for merge based on a few things -

while this doesn't solve everything from the conversation in multigather CSV output uses signature filename as basename. #2328, it detects and fixes some existing bad behavior
several people over in Taxonomic labels for individual contigs in metagenomic assembly #2816 really want to use -U
merging latest was really annoying and I don't want to do that again

and finally, @bluegenes is doing different experiments in re output files with fastmultigather over in sourmash-bio/sourmash_plugin_branchwater#197, and so I think we have routes to explore better output over there.

It might be that a long-term future for multigather is to deprecate it in sourmash and direct people to fastmultigather, which is actually much faster than multigather.

ctb · 2024-02-29T15:31:28Z

src/sourmash/commands.py

-                    remaining_mh = remaining_query.minhash.to_mutable()
-                    remaining_mh += noident_mh.downsample(scaled=remaining_mh.scaled)
-                    remaining_query.minhash = remaining_mh
+            output_unassigned = output_base + f".unassigned{args.extension}"


Removing the open fixes a bug I introduced in

sourmash/src/sourmash/commands.py

Line 1085 in 2b86b0d

with open(output_unassigned, 'wt') as fp:

, where I mangle tessa's original code by using SaveSignaturesToLocation on an already opened filename. The switch to using args.extension revealed the bug.

All the other changes here are just indentation based.

ctb · 2024-02-29T15:32:15Z

src/sourmash/commands.py


-                with SaveSignaturesToLocation(output_unassigned) as save_sig:
-                    # CTB: note, multigather does not save abundances


multigather does now save abundances, so the comment was false ;)

ctb · 2024-02-29T15:32:38Z

tests/test_sourmash.py

@@ -4886,7 +4886,10 @@ def test_multigather_metagenome_output(runtmp):
    cmd = cmd.split(" ")
    c.run_sourmash(*cmd)

-    output_csv = runtmp.output("-.csv")


fixa da bug

ctb · 2024-02-29T15:32:55Z

tests/test_sourmash.py

@@ -4916,15 +4919,17 @@ def test_multigather_metagenome_output_outdir(runtmp):
    cmd = cmd.split(" ")
    c.run_sourmash(*cmd)

-    output_csv = runtmp.output("savehere/-.csv")


fixa da bug

ctb · 2024-02-29T15:33:13Z

tests/test_sourmash.py

    testdata_glob = utils.get_test_data("gather/GCF*.sig")
    testdata_sigs = glob.glob(testdata_glob)
-
-    utils.get_test_data("gather/combined.sig")


removed no-op

ctb · 2024-02-29T15:33:25Z

tests/test_sourmash.py

@@ -5050,11 +5062,7 @@ def test_multigather_metagenome_query_with_sbt_addl_query(c):

    assert os.path.exists(c.output("gcf_all.sbt.zip"))

-    another_query = utils.get_test_data(


removed redundancy

ctb · 2024-02-29T15:33:48Z

tests/test_sourmash.py


+    assert (


changed behavior

ctb · 2024-02-29T15:34:06Z

tests/test_sourmash.py

    c.run_sourmash(*cmd)

    out = c.last_result.out
    print(out)
    err = c.last_result.err
    print(err)

-    assert "conducted gather searches on 13 signatures" in err
+    assert (


changed behavior

ctb · 2024-02-29T16:25:59Z

@sourmash-bio/devs ready for review & merge! I've tried to clearly annotate the changed behavior and so on in the tests... good luck!

bluegenes · 2024-02-29T20:49:49Z

src/sourmash/commands.py

+                output_base = query.md5sum()
+            elif args.output_add_query_md5sum:
+                # Uniquify the output file if all signatures were made from the same file (e.g. with --singleton)
+                assert query_filename and query_filename != "-"  # first branch
+                output_base = os.path.basename(query_filename) + "." + query.md5sum()


what happens in the case of duplicated md5sums? I assume this will be much more likely with short sequences that are sketched with --singleton

Then the output_base will be flagged as a duplicate - which I think is appropriate, if not ideal ;). People can choose to force allow override, or... do something else.

bluegenes

Other than potential issues from duplicated md5sums, I think this looks ok.

duplicated md5sums would imply that the hashes are the same anyway, so really the issue would present as a non 1:1 query:outputfile issue -- all results would be available, just in slightly fewer files than 1 per query.

ctb · 2024-02-29T21:18:38Z

I'll merge then! Thanks!

ctb · 2024-02-29T21:18:55Z

😮‍💨

# Release notes for sourmash v4.8.7 Note: This release changes the way `sourmash multigather` names output files in some situations. Please see #2722 for details. Minor new features: * support proper manifest creation with `--relpath` for `sig check` and `sig collect` (#3054) * fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` (#2722) * enable loading lineages from annotated gather with match_name instead of name (#3078) Bug fixes: * fix output for `sketch ... --singleton` (#3066) * fix `calculate_gather_stats` `threshold=0` bug (#3052) Cleanup and documentation updates: * adjust protein ksize for record/manifest (#3019) * Resolve `sourmash gather --help` issue (#3032) * rework the manifest documentation; do misc cleanup (#3027) * add branchwater web to docs (#3018) Developer updates: * make core Manifest booleans python compatible (core) (#3007) * safer ksize selection while still accommodating k=k*3 (#3028) * fix clippy beta issues (#3088) * tell dependabot to ignore upgrades to `byteorder`, `chrono`, `once_cell`, and `wasm-bindgen` (#3065) * update rust changelog for r0.13.0 in preparation for release (#3033) * Allow changing storage location for a collection in RevIndex (#3015) * Fix tox and nix configs so all tox tests execute correctly (#2992) * Calculate all gather stats in rust; use for rocksdb gather (#2943) * bump screed req to 1.1.3 (#3067) * bump to v4.8.7-dev (#2989) Dependabot updates: * Bump DeterminateSystems/magic-nix-cache-action from 1 to 3 (#2994) * Bump DeterminateSystems/magic-nix-cache-action from 3 to 4 (#3085) * Bump DeterminateSystems/nix-installer-action from 4 to 9 (#2995) * Bump DeterminateSystems/nix-installer-action from 9 to 10 (#3083) * Bump chrono from 0.4.33 to 0.4.34 (#3000) * Bump conda-incubator/setup-miniconda from 3.0.1 to 3.0.2 (#3046) * Bump conda-incubator/setup-miniconda from 3.0.2 to 3.0.3 (#3057) * Bump histogram from 0.9.0 to 0.9.1 (#3002) * Bump itertools from 0.12.0 to 0.12.1 (#3043) * Bump log from 0.4.20 to 0.4.21 (#3062) * Bump num-iter from 0.1.43 to 0.1.44 (#2997) * Bump pypa/cibuildwheel from 2.16.5 to 2.17.0 (#3084) * Bump rayon from 1.8.1 to 1.9.0 (#3058) * Bump roaring from 0.10.2 to 0.10.3 (#3014) * Bump serde from 1.0.196 to 1.0.197 (#3045) * Bump serde_json from 1.0.113 to 1.0.114 (#3044) * Bump tempfile from 3.10.0 to 3.10.1 (#3059) * Bump thiserror from 1.0.56 to 1.0.57 (#2999) * Bump thiserror from 1.0.57 to 1.0.58 (#3082) * Bump wasm-bindgen from 0.2.91 to 0.2.92 (#3060) * Bump wasm-bindgen-test from 0.3.40 to 0.3.41 (#2996) * Bump wasm-bindgen-test from 0.3.41 to 0.3.42 (#3063) * Bump web-sys from 0.3.67 to 0.3.68 (#2998) * Bump web-sys from 0.3.68 to 0.3.69 (#3061) * Revert "Bump wasm-bindgen from 0.2.91 to 0.2.92 (#3060)" (#3064) * Update asv to 0.6.2 (#3025) * Update pytest requirement from <8.1.0,>=6.2.4 to >=6.2.4,<8.2.0 (#3075)

olgabot and others added 5 commits May 28, 2022 13:23

Merge branch 'latest' of https://github.com/sourmash-bio/sourmash int…

3e4a2d3

…o ctb_update_2065

fix merge mistake

db99f6d

Merge branch 'latest' of https://github.com/sourmash-bio/sourmash int…

4f73ab7

…o ctb_update_2065

update 2065

29b7acf

ctb changed the title ~~WIP: fix multigather output by adding md5sum~~ WIP: fix multigather output by adding md5sum along with -U/--output-add-query-md5sum Aug 19, 2023

ctb added 3 commits August 19, 2023 09:03

deal with overwriting in tests

096e116

add tests for detecting overwrite

8a54cc5

fix filename == '-' issue

5fcd12a

ctb mentioned this pull request Aug 19, 2023

multigather CSV output uses signature filename as basename. #2328

Open

Base automatically changed from ctb_update_2065 to olgabot-patch-2 August 21, 2023 00:56

ctb added 8 commits August 20, 2023 19:47

Merge branch 'latest' of https://github.com/sourmash-bio/sourmash int…

4e148a0

…o olgabot-patch-2

Merge branch 'olgabot-patch-2' of https://github.com/sourmash-bio/sou…

6a8bf5f

…rmash into fix_multigather_output

add some docs

2c6b194

add new option + tests + docs

d6dd054

cleanup on aisle 10

0b7b7f6

more cleanup of tests

53e7241

test -E/--extension

3d0467e

Merge branch 'latest' of https://github.com/sourmash-bio/sourmash int…

5905e18

…o fix_multigather_output

ctb mentioned this pull request Oct 19, 2023

Taxonomic labels for individual contigs in metagenomic assembly #2816

Closed

ctb added 4 commits October 19, 2023 08:01

typo

eba4d2a

Merge branch 'latest' of https://github.com/sourmash-bio/sourmash int…

556bf3f

…o fix_multigather_output_merge

fix multigather argparse

7296cb7

cleanup from merge

e8bb12f

ctb changed the base branch from olgabot-patch-2 to latest February 28, 2024 14:33

ctb mentioned this pull request Feb 28, 2024

MRG: Uniquify csv output from multigather #2065

Closed

pre-commit-ci bot and others added 2 commits February 28, 2024 14:35

[pre-commit.ci] auto fixes from pre-commit.com hooks

3589780

for more information, see https://pre-commit.ci

text fix/cleanup attempt 1

7878886

ctb added 4 commits February 29, 2024 06:53

fix multigather test

8b3f708

fix up documentation a bit

d436bca

fix remaining @ctb

4d3059e

add tests for -U

839f4ef

ctb changed the title ~~WIP: fix multigather output by adding md5sum along with -U/--output-add-query-md5sum~~ MRG: fix multigather output by adding md5sum along with -U/--output-add-query-md5sum Feb 29, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

1265d82

for more information, see https://pre-commit.ci

ctb mentioned this pull request Feb 29, 2024

deprecate sourmash multigather for 5.0 #1614

Open

ctb commented Feb 29, 2024

View reviewed changes

tests/test_sourmash.py

assert (

Copy link

Contributor Author

ctb Feb 29, 2024

Choose a reason for hiding this comment

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

changed behavior

ctb commented Feb 29, 2024

View reviewed changes

bluegenes reviewed Feb 29, 2024

View reviewed changes

bluegenes approved these changes Feb 29, 2024

View reviewed changes

bluegenes mentioned this pull request Feb 29, 2024

optionally output matches and unassigned from fastmultigather sourmash-bio/sourmash_plugin_branchwater#258

Open

ctb merged commit c7a1265 into latest Feb 29, 2024
41 checks passed

ctb deleted the fix_multigather_output branch February 29, 2024 21:18

This was referenced Mar 20, 2024

multigather individual contigs from one file #3089

Closed

MRG: 4.8.7 release branch #3090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG: fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` #2722

MRG: fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` #2722

ctb commented Aug 19, 2023 •

edited

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading

ctb commented Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb Feb 29, 2024

ctb commented Feb 29, 2024

bluegenes Feb 29, 2024

ctb Feb 29, 2024

bluegenes left a comment

ctb commented Feb 29, 2024

ctb commented Feb 29, 2024


		with SaveSignaturesToLocation(output_unassigned) as save_sig:
		# CTB: note, multigather does not save abundances

		@@ -5050,11 +5062,7 @@ def test_multigather_metagenome_query_with_sbt_addl_query(c):

		assert os.path.exists(c.output("gcf_all.sbt.zip"))

		another_query = utils.get_test_data(

MRG: fix multigather output by adding md5sum along with -U/--output-add-query-md5sum #2722

MRG: fix multigather output by adding md5sum along with -U/--output-add-query-md5sum #2722

Conversation

ctb commented Aug 19, 2023 • edited Loading

codecov bot commented Aug 19, 2023 • edited Loading

Codecov Report

ctb commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctb commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluegenes left a comment

Choose a reason for hiding this comment

ctb commented Feb 29, 2024

ctb commented Feb 29, 2024

MRG: fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` #2722

MRG: fix `multigather` output by adding md5sum along with `-U/--output-add-query-md5sum` #2722

ctb commented Aug 19, 2023 •

edited

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading