Exclude future dates

[kairstenfay]

Exclude future dates with augur filter --max-date.

This PR previously concatenated an automatically generated exclude.txt file created by ncov-ingest with the manually curated one in this repo, but now those concatenation steps are no longer necessary. Down the road, we may want to incorporate automatic exclusions, so don't drop the commits here.

Depends on nextstrain/ncov-ingest#43

Related issue(s)

nextstrain/ncov-ingest#33

[kairstenfay]

ncov-ingest: warn about future dates

[tsibley]

@trvrb This arose from a suggestion of mine in nextstrain/ncov-ingest#33. The goal is to reduce manual annotation curation while making what's excluded very transparent in the same way that config/exclude.txt is. I agree that the build will need to work for everyone, of course. The fetch from s3 can either be optional or we could even make that object public. I also see ncov-ingest as ideally ending up incorporated into this repo as part of the build (esp. now that it's public).

[kairstenfay]

@trvrb does having an optional fetch or a publicly available S3 bucket where the automatically generated exclude.txt is stored seem like workable solutions here?

[kairstenfay]

I also see ncov-ingest as ideally ending up incorporated into this repo as part of the build (esp. now that it's public).

That would be fantastic 😄

[tsibley]

@kairstenfay Would you rebase this onto the latest master while you're making the tweaks below?

[tsibley]

Instead of modifying the hardcoded config/exclude.txt in place, this rule would be better if it took config["files"]["exclude"] as its input and produced a separate file as its output, something like results/exclude.txt maybe. Then the filter rule immediately following would be adjusted to take rules.combine_exclude_files.output.exclude as its input.

This avoids modifying a git-tracked file, which will affect the git repo's dirty/clean status, and avoids hardcoding the input base excludes file.

The command should probably also make sure when it appends the remote excludes file that there's a trailing newline in the base file. I've noticed that not everyone's editor ensures this and some files are missing the trailing newline (which breaks a lot of assumptions that Unix tools make).

[kairstenfay]

Updated in the latest commit

edit: Oops, the snakemake input/output changes only. Still need to ensure trailing newline.

[tsibley]

Ah, and as you noted in the related ncov-ingest PR, we'll also need to make the combining rule optional.

[kairstenfay]

Ah, and as you noted in the related ncov-ingest PR, we'll also need to make the combining rule optional.

@tsibley Is the download rule no longer optional? I recall it was at one point but download fails, breaking the Snakemake build, if I don't provide AWS credentials.

[kairstenfay]

Perhaps if not config["connect_to_s3"], we don't want to do any of the deployment and error handling setup above this line.

[kairstenfay]

@tsibley in my latest commit, I propose a solution to making S3 downloads and deploys optional. It required creating a new config file, profiles/nextstrain/s3.yaml, in order to set the boolean connect_to_s3 variable to a different value in the default and nextstrain profiles.

[kairstenfay]

I just rebased onto master and pushed a new commit that excludes future dates using augur filter --max-date as suggested by @joverlee521 in nextstrain/ncov-ingest#43.

I updated the PR title and initial message to reflect these changes. I did not drop the exclude.txt concatenation commit from this PR, because down the road, we may have a reason to produce an automatically generated exclude.txt file (e.g. see nextstrain/ncov-ingest#45).

[tsibley]

@kairstenfay Would you mind pulling out the --max-date commit and merging just that for now? I do think we should keep the other commits around in case of future work but would prefer to leave them in an open PR until we need them. Generally, I find that minimizing unused code in the main branch makes a project easier to work with and understand.

[kairstenfay]

@kairstenfay Would you mind pulling out the --max-date commit and merging just that for now? I do think we should keep the other commits around in case of future work but would prefer to leave them in an open PR until we need them. Generally, I find that minimizing unused code in the main branch makes a project easier to work with and understand.

That makes perfect sense. I created #437 that contains the commits now dropped from this PR.

[tsibley]

Something went wrong with the rebase, as the definition of input.date disappeared for this rule. In any case, I think better to make date a param instead of an input.

[kairstenfay]

Ah yes I made an error in rebasing. params.date is defined now.

I'm curious, what is the distinction between input and params in Snakemake? To me it feels arbitrary because we use entries from config in both. I tried looking through the Snakemake docs but couldn't find anything on this.

After talking with @joverlee521 , my understanding is it that input generally is for files and params is for other values.

[tsibley]

Yeah, I concur with @joverlee521. Snakemake describes the motivation for params as well in its docs: https://snakemake.readthedocs.io/en/stable/snakefiles/rules.html#non-file-parameters-for-rules

[kairstenfay]

Hmm... augur filter with --max-date is no longer working for me locally when I run snakemake. I wonder if you all are experiencing the same?

[kairstenfay]

I get this error, but only when I include --max-date:

Error in rule filter:
    jobid: 137
    output: results/filtered.fasta
    log: logs/filtered.txt (check log file(s) for error message)
    shell:
        
        augur filter             --sequences data/sequences.fasta             --metadata data/metadata.tsv             --include config/include.txt             --max-date 2020-06-16             --exclude config/exclude.txt             --exclude-where division='USA' date='2020' date='2020-01-XX' date='2020-02-XX' date='2020-03-XX' date='2020-04-XX' date='2020-05-XX' date='2020-06-XX' date='2020-01' date='2020-02' date='2020-03' date='2020-04' date='2020-05' date='2020-06'            --min-length 27000             --output results/filtered.fasta 2>&1 | tee logs/filtered.txt
        
        (one of the commands exited with non-zero exit code; note that snakemake uses bash strict mode!)

Digging through the augur filter code it looks like --max-date should be a float?

[joverlee521]

Digging through the augur filter code it looks like --max-date should be a float?

I get the same error. It works when I hardcode the max-date to 2020.06. Why is this expected to be a float???

[emmahodcroft]

I believe this may have only ever have been intended to be used for years.... Might be worth peeking at the code to see if this is the case. Sorry if this is the case and I didn't remember this earlier...! Otherwise, there are some functions in augur that will convert to decimal date, I think (or Treetime), which could maybe be used to convert today into a decimal

[tsibley]

Oof. Dates as floats are pretty common in Augur-land (and have caused consternation in the past). I see two options, which aren't mututally exclusive:

1. Convert date.today() to Augur's style of floating point date in the Snakefile before passing to --max-date.

2. Extend Augur's --min-date and --max-date to also accept a YYYY-MM-DD string. Internally, it would still likely convert to a floating point date for the comparison, but as Emma points out, functions exist to do that already. This should be backwards compatible and could be added relatively easily.

[tsibley]

from treetime.utils import numeric_date
…
        date: numeric_date(date.today())

should do the trick for the first option.

[kairstenfay]

I believe this may have only ever have been intended to be used for years.... Might be worth peeking at the code to see if this is the case. Sorry if this is the case and I didn't remember this earlier...! Otherwise, there are some functions in augur that will convert to decimal date, I think (or Treetime), which could maybe be used to convert today into a decimal

Thank you, and no worries at all... I swear --max-date used to work for me locally, but I could be misremembering.

[kairstenfay]

Oof. Dates as floats are pretty common in Augur-land (and have caused consternation in the past). I see two options, which aren't mututally exclusive:

1. Convert `date.today()` to Augur's style of floating point date in the Snakefile before passing to `--max-date`.

2. Extend Augur's `--min-date` and `--max-date` to also accept a `YYYY-MM-DD` string. Internally, it would still likely convert to a floating point date for the comparison, but as Emma points out, functions exist to do that already. This should be backwards compatible and could be added relatively easily.

Thanks for laying out these options, Tom. I'm leaning towards extending augur in a backwards compatible way, because it would be nice for augur filter to accept ISO 8601 dates which many users (like myself) might expect.

[kairstenfay]

Thanks for laying out these options, Tom. I'm leaning towards extending augur in a backwards compatible way, because it would be nice for augur filter to accept ISO 8601 dates which many users (like myself) might expect.

Ok, I'll mark option 2 as "aspirational" for now. I filed an issue at nextstrain/augur/issues/567.

As @tsibley mentioned, the two solutions he proposed are not mutually exclusive. So, the easiest thing to do for now is convert today's date to a numeric date with treetime.utils. I implemented the first solution Tom proposed and fixed it up into the single commit here.

[emmahodcroft]

Thank you guys, and thanks @kairstenfay for making an issue in augur! I think this would be great to expand augur filter to - but yes, probably easiest not to hold up this PR in the meantime :)