IMP: Do no allow duplicate records in DNAFASTAFormat #220
Conversation
There was a problem hiding this comment.
Keep track of all lines all ids occur on and do not error out as soon as a duplicate is found. Instead wait until the validation completes (assuming it doesn't run into any other errors) and report any and all duplicates found while validating along with the lines they occurred on (this seems like overkill to me)
I am fine with not worrying about this for now, what is more important to the original issue is to get this behavior rolled into the aligned format, too.
q2_types/feature_data/_format.py
Outdated
| @@ -161,6 +162,11 @@ def _validate_lines(self, max_lines): | |||
| raise ValidationError('Multiple consecutive IDs ' | |||
| 'starting on line ' | |||
| f'{line_number-1!r}') | |||
| if line in ids: | |||
There was a problem hiding this comment.
One problem with this is that it treats the entire header line as the ID. I believe that we need to treat everything between the > and the first instance of whitespace ( , if any) as the ID. Characters after the space are part of the record's description field, and are not subject to the ID uniqueness rules.
@ebolyen, can you please confirm this for me?
There was a problem hiding this comment.
Thanks @ebolyen! @Oddant1, now would be a good time to fix the error message on https://github.com/qiime2/q2-types/pull/220/files#diff-c7c5016b403f1d6d2fb45764b58ae9d6R153 to indicate this situation, too (technically the ID doesn't start with >, the refline for the record starts with >.
There was a problem hiding this comment.
Are they allowed to have a space immediately after the >? We don't currently check for that at all.
There was a problem hiding this comment.
I think the most comprehensive resource is here:
http://scikit-bio.org/docs/0.5.5/generated/skbio.io.format.fasta.html#module-skbio.io.format.fasta
Have fun... 🎉
There was a problem hiding this comment.
I guess if they had a space immediately after the > their id would start with a space and that would be silly, so I suppose they can't do that. That seems to be what that format definition implies.
There was a problem hiding this comment.
I would probably interpret it as a "null" ID, but in any case, that is almost certainly a formatting error and we should probably avoid accepting it.
Addresses #198 at least for
DNAFASTAFormat. After this is done, maybe I can makeAlignedDNAFASTAFormatvalidate in a similar manner toDNAFASTAFormat. currently all this does is make a set of all encountered ids and raise aValidationErrorif it finds any overlaps. This is basically the most basic initial implementation I could think of so we could potentially create a more robust implementation from here, some ideas for that.Thoughts?