curate apply-geolocation-rules: use default rules and custom rules #1745
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1745 +/- ##
==========================================
+ Coverage 73.06% 73.11% +0.04%
==========================================
Files 79 79
Lines 8335 8350 +15
Branches 1700 1704 +4
==========================================
+ Hits 6090 6105 +15
Misses 1958 1958
Partials 287 287 ☔ View full report in Codecov by Sentry. |
The new geolocation_rules.tsv does not exist yet, so the linkcheck fails with a 404 status code. Temporarily ignore this URL until we do a new release of Augur that includes the file. <#1745 (comment)>
| [record.get(field, '') for field in location_fields], | ||
| args.case_sensitive) | ||
|
|
||
| # If no custom rules are applied, then try to apply the default rules |
There was a problem hiding this comment.
[scanning through code] Is this the bit that resolves #1648?
There was a problem hiding this comment.
Yup! I've tried to make it straight-forward that a record will either get changed by the custom rules or the default rules.
There was a problem hiding this comment.
👍 I imagine you can still get circular-errors within the custom rules, but this seems acceptable.
There was a problem hiding this comment.
Yeah, I'd consider circular-errors within the custom rules a user error. Hmm, maybe we should make it easier to check for cyclic rules in the rules file...
| class NoGeolocationRulesProvidedError(AugurError): | ||
| pass |
There was a problem hiding this comment.
[not a request for changes] I'm curious, what's the benefit of a new error vs AugurError?
There was a problem hiding this comment.
Probably overkill in this case, but usually the main benefit for me is being able to catch specific errors.
joverlee521
force-pushed
the
default-geolocation-rules
branch
from
96298c0 to
58eade9
Compare
`--geolocation-rules` is now optional and acts as custom rules that have precedence over the default geolocation rules in Augur. The custom rules are applied first and if none of the custom rules are applied to a record, then use the default rules. This means that if the custom rules have wildcards, then all locations that match the wildcards will no longer use the default rules. This is best demonstrated in the Cram test included in this commit.
Allow users to completely ignore Augur's default geolocation rules without having to override all of them in the custom rules.
In the case where user is using the `--no-default-rules` flag and have not provided custom rules via the `--geolocation-rules` option, the records will pass through without changes. Rather than have this be a no-op, I think this is a user error and should fail loudly.
The new geolocation_rules.tsv does not exist yet, so the linkcheck fails with a 404 status code. Temporarily ignore this URL until we do a new release of Augur that includes the file. <#1745 (comment)>
joverlee521
force-pushed
the
use-geolocation-rules
branch
from
5e7e10d to
02ad675
Compare
joverlee521
force-pushed
the
use-geolocation-rules
branch
from
02ad675 to
430e70e
Compare
Remove the use of the ncov-ingest geolocation rules since Augur now uses the built-in geolocation rules by default. Depends on the release of <nextstrain/augur#1745>
Remove the use of the ncov-ingest geolocation rules since Augur now uses the built-in geolocation rules by default. Depends on the release of <nextstrain/augur#1745>
Description of proposed changes
This PR is based on #1744
Improvements to
augur curate apply-geolocation-rules:--geolocation-ruleshave precedence over the default rules--no-default-rulesflagRelated issue(s)
Resolves #1648
Checklist