Implement weighted sampling

[victorlin]

Description of proposed changes

This new option takes a weights file to determine group sizes. Summarized by the help text:

augur/augur/filter/__init__.py

Lines 73 to 97 in 9a2090b

	subsample_group.add_argument('--group-by-weights', type=str, metavar="FILE", help="""
	TSV file defining weights for grouping. Requirements:
	(1) Lines starting with '#' are treated as comment lines.
	(2) The first non-comment line must be a header row.
	(3) There must be a numeric ``weight`` column (weights can take on any
	non-negative values).
	(4) Other columns must be a subset of columns used in ``--group-by``,
	with combinations of values covering all combinations present in the
	metadata.
	(5) This option only applies when ``--group-by`` and
	``--subsample-max-sequences`` are provided.
	(6) This option cannot be used with ``--no-probabilistic-sampling``.
	Notes:
	(1) Any ``--group-by`` columns absent from this file will be given equal
	weighting across all values *within* groups defined by the other
	weighted columns.
	(2) An entry with the value ``default`` under all columns will be
	treated as the default weight for specific groups present in the
	metadata but missing from the weights file. If there is no default
	weight and the metadata contains rows that are not covered by the
	given weights, augur filter will exit with an error.
	""")

Related issue(s)

• Allow weighted subsampling #1318

Notable comment threads

• How to handle a mix of weighted and unweighted columns? Apply weights within unweighted groups
• How to handle weighted groups that don't appear in metadata? Drop them
• Add an option to show number of outputted sequences? Done
• How to handle over-targeting group sizes with only weighted columns? Added warnings in output
• How to handle over-targeting group sizes with a mix of weighted and unweighted columns? Added warnings in output
• Clarify interaction with probabilistic sampling Done

Checklist

• Prototype usage in an existing workflow: Use weighted sampling for Asia builds ncov#1106
• Automated checks pass
• Check if you need to add a changelog message
• Check if you need to add tests (added)
• Check if you need to update docs
• Create issue for FIXME: filter: Make under-sampling more apparent #1590

[victorlin]

This should be in a working state, at least based on limited testing in subsample-weighted.t. Planning to test using #1444 + nextstrain/ncov#1102 to get a feel for how add an additional weights.tsv file fits in a pathogen repo and make sure it works as expected on real data.

---

UPDATE: first testing in ncov independently of augur subsample

[jameshadfield]

This is going to be really powerful @victorlin, nice work. I tested this out a little bit, but haven't gone through every part of it. There's a few aspects that this brought up for which I'll think through a little more, but I think the overall direction is sound.

[genehack]

Having just read this from scratch, and hoping to help it get moving again, it seems like the remaining outstanding discussion items are:

#1 adding an (optional?) output with requested and calculated group sizes – which seems like a pretty valuable debugging tool to me, but maybe non-blocking for this particular PR
#2 validating that the current behavior when a group-by group is empty is desired convo continued in second comment – seems like this is the case but maybe a bit more explicit validation/commentary would be good?

Did I miss anything @victorlin ?

[victorlin]

Thanks for bumping this @genehack. I added a comment on the thread for (1). Testing and discussion in nextstrain/ncov#1106 should address (2) and impact the overall direction of this PR. I want to make sure we have at least one solid use case for this feature.

[codecov]

Codecov Report

Attention: Patch coverage is 98.72611% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.98%. Comparing base (d8faf01) to head (9a2090b).
Report is 120 commits behind head on master.

Files with missing lines	Patch %	Lines
augur/filter/weights_file.py	94.59%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1454      +/-   ##
==========================================
+ Coverage   70.51%   70.98%   +0.46%     
==========================================
  Files          78       79       +1     
  Lines        8107     8244     +137     
  Branches     1966     2002      +36     
==========================================
+ Hits         5717     5852     +135     
- Misses       2100     2101       +1     
- Partials      290      291       +1     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

new changes

[joverlee521]

I haven't fully grokked what's going on in get_weighted_group_sizes, but the behaviors described in the functional tests make sense to me.

Once this feature is released, it definitely deserves it's own section in the Filtering and Subsampling docs.

[jameshadfield]

[general question about test best practices]

I commonly use cram --keep-tmpdir and then inspect the files themselves to understand the tests & thus behaviour of augur. This test overwrites files (e.g. weights.tsv) so that approach doesn't work. It also seems like it'd hide bugs where a command fails, but exits code 0, and thus we're not using the version of the file we think we are. Thoughts on using weights1.tsv, weights2.tsv etc?

[victorlin]

I've added more specific file names: b7fc345.

The other approach which I've found better for debugging tests is to split the test file into multiple files. However, in this case it's better to keep all in the same file to share the temporary metadata.tsv.

It also seems like it'd hide bugs where a command fails, but exits code 0, and thus we're not using the version of the file we think we are.

I assume "command" = "Augur command", but I'm having trouble understanding your scenario.

• Weights files are created separately from the Augur commands.
• Augur command failures will exit with a nonzero exit code and be shown as a test failure.

[victorlin]

[@joverlee521] I haven't fully grokked what's going on in get_weighted_group_sizes

I've split this function up into smaller parts and added types to make it more readable: c305fe8

[trvrb]

@victorlin: This is great! I appreciate all the documentation and tests. I'm good for this to be merged as it stands. However, I do think we should follow up shortly with a separate PR that implements support for --probabilistic-sampling + --group-by-weights per #1454 (comment). I was confused. Nothing more to be done with probabilistic sampling.

[victorlin]

Finished addressing all conversations and cleaned up commits. I'm going to re-run trial builds once more on nextstrain/ncov#1106 with latest changes then merge this tomorrow if all looks good.