Add a parameter to augur parse to specify a different record ID for output sequences FASTA #1403
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
85bf1bf
to
114c901
Compare
j23414
changed the title
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
114c901
to
d1aef81
Compare
victorlin
requested changes
Jan 29, 2024
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
0e07091
to
7b45b2f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1403 +/- ##
=======================================
Coverage 67.46% 67.47%
=======================================
Files 69 69
Lines 7484 7474 -10
Branches 1840 1840
=======================================
- Hits 5049 5043 -6
+ Misses 2161 2159 -2
+ Partials 274 272 -2 ☔ View full report in Codecov by Sentry. |
huddlej
reviewed
Jan 31, 2024
augur/parse.py
Outdated
| strain_key = 'strain' | ||
| else: | ||
| strain_key = None | ||
| for possible_id in [args.output_id_field, DEFAULT_ID_COLUMNS]: |
There was a problem hiding this comment.
I like @victorlin's suggested alternative to this block, but I wanted to mention that this code on line 167 doesn't do what you want it to do, @j23414. Instead of looping through a list of the user-provided output id field and the default ids, this will loop through the user-provided field and the tuple of defaults:
>>> DEFAULT_ID_COLUMNS = ("strain", "name")
>>> output_id_field = "accession"
>>> for possible_id in [output_id_field, DEFAULT_ID_COLUMNS]:
... print(possible_id)
...
accession
('strain', 'name')You could make this work as expected a couple of ways including iterable unpacking with the * operator or list concatenation.
# Iterable unpacking with *
>>> for possible_id in [output_id_field, *DEFAULT_ID_COLUMNS]:
... print(possible_id)
...
accession
strain
name
# List concatenation
>>> for possible_id in [output_id_field] + list(DEFAULT_ID_COLUMNS):
... print(possible_id)
...
accession
strain
name
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
7b45b2f
to
8367e59
Compare
victorlin
requested changes
Feb 2, 2024
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
2 times, most recently
from
fc6c6cc
to
f587119
Compare
victorlin
requested changes
Feb 7, 2024
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
40e9a00
to
9810855
Compare
victorlin
approved these changes
Feb 7, 2024
victorlin
reviewed
Feb 8, 2024
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
3 times, most recently
from
e41dbb5
to
d313f58
Compare
Originally augur parse first checks for 'name' then 'strain' and then the first field to figure out the sequence id to use. This test checks the default id column order.
During augur parse, instead of hardcoding a check for a 'name' field and then a 'strain' field to use as a sequence ID, reuse and check against the io.metadata DEFAULT_ID_COLUMNS list to be more consistent with the rest of augur. This will result in a breaking behavior change in that parse is now checking for 'strain' before 'name' as the default ID column. However, the argument to be more consistent with the rest of augur is that the DEFAULT_ID_COLUMNS list is already used in other parts of augur and augur parse should be no different.
Include a `--output-id-field` parameter in augur parse to indicate the ID
field for the output sequences file, such as using "accession" instead of
"strain".
It is important to note that the `--output-id-field` parameter is not required,
and augur parse will fall back to the DEFAULT_ID_COLUMNS (e.g. ('strain','name'))
if it is not present. If none of the DEFAULT_ID_COLUMNS are present in the fields,
fall back to using the first field.
The `--output-id-field` parameter is designed to accept a single field name,
not multiple. User are required to provide a `--fields col1 col2 strain accession`
argument in the same invocation. It seems reasonable to expect the user to
choose a specific field name for the ID.
To prevent unintended behaviors, if `--output-id-field` is not present in
`--fields` (e.g., due to a typo), augur parse will raise an error instead of
falling back to DEFAULT_ID_COLUMNS.
… future. Instead of a refactor and breaking change from 8dc0694, this commit adds a deprecation warning to the `parse` step that the default `id` field used will be reordered from ('name', 'strain') to ('strain', 'name'). This will give users time to update their scripts and workflows with an `--output-id-field 'name'` before the change is made. co-authored-by: Victor Lin <13424970+victorlin@users.noreply.github.com>
j23414
force-pushed
the
parse-output-metadata-id-columns
branch
from
da2a23c
to
9d96cad
Compare
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
As discussed in #1402, include a parameter in augur parse to indicate the ID field for the output sequences file, such as using "accession" instead of "strain" or "name."
Open to other suggestions for parameter names or let me know if I missed adding a test somewhere.
Related issue(s)
--metadata-id-columnsparam to augur parse? #1402Checklist