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

Add apply-geolocation-rules #2

Closed
wants to merge 1 commit into from
Closed

Conversation

jameshadfield
Copy link
Member

@jameshadfield jameshadfield commented Jul 11, 2023

See the commit message for the other occurrences of this script, and the changes between the different versions.

This was tested in both the hepatitisB and monkeypox ingest pipelines, with no changes to the resulting outputs.

I've made an issue to discuss the specifics of using git subtree to create & test this commit.

This moves the script to the vendored directory, which is a git subtree
of https://github.com/nextstrain/ingest (currently at branch
apply-geolocation-rules). The file suffix is removed to match how it
appears in the other repos which use it.

As per [Overview of duplicated scripts](#1)
this script also appears in:
* [monkeypox](https://github.com/nextstrain/monkeypox/blob/a1f0d7b757d323d87edcbe61c6c5ccfbdf47722c/ingest/bin/apply-geolocation-rules)
* [rsv](https://github.com/nextstrain/rsv/blob/ba171f4a43110382c38b6154be3febd50408d7bf/ingest/bin/apply-geolocation-rules)
* [dengue, branch new_ingest](https://github.com/nextstrain/dengue/blob/247b2fd897361f2548627de1d97d45fae4115c5c/ingest/bin/apply-geolocation-rules)

All three of those scripts are identical to each other. The script vendored
here contains two code changes (whitespace removed from diffs):

**Ignore comment lines in the location-rules TSV**
```diff
< if line.lstrip()[0] == '#':
---
> if line.strip()=="" or line.lstrip()[0] == '#':
```

**Allow fields to be missing from the input NDJSON**

The script previously mandated that the input NDJSON had all four
fields (region/country/division/location). This is relaxed here, with
an empty string used if the field is not present.

```diff
< annotated_values = transform_geolocations(geolocation_rules, [record[field] for field in location_fields])
---
> annotated_values = transform_geolocations(geolocation_rules, [record.get(field, '') for field in location_fields])
```
@victorlin
Copy link
Member

Instead of pushing the script + changes directly from the hepatitisB repo, I suggest copying them over separately, for example: #4 + #5

@jameshadfield
Copy link
Member Author

I'm happy for #4 + #5 to go through instead of this

@victorlin
Copy link
Member

Closing in favor of #4, #5.

@victorlin victorlin closed this Aug 1, 2023
@victorlin victorlin deleted the apply-geolocation-rules branch August 1, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants