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

BUG: Properly validate FASTA files #214

Merged
merged 5 commits into from
Jun 19, 2019

Conversation

Oddant1
Copy link
Member

@Oddant1 Oddant1 commented Jun 6, 2019

closes #187

@thermokarst thermokarst changed the title BUG: Properly validate FASTA files closes#187 BUG: Properly validate FASTA files Jun 6, 2019
@thermokarst thermokarst added this to In Review in 2019.7 via automation Jun 6, 2019
@ebolyen ebolyen self-assigned this Jun 6, 2019
Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks, and a potential strategy for testing the buffer boundaries (which would be good to demonstrate we 100% understand the way that is working).

q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Show resolved Hide resolved
@ebolyen ebolyen moved this from In Review to In Development in 2019.7 Jun 6, 2019
@ebolyen ebolyen assigned Oddant1 and unassigned ebolyen Jun 6, 2019
@ebolyen ebolyen moved this from In Development to In Review in 2019.7 Jun 13, 2019
@ebolyen ebolyen assigned ebolyen and unassigned Oddant1 Jun 13, 2019
Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed much better! Maybe we should think about how we could generalize this for other formats in the back of our heads (obviously not for this PR).

q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/_format.py Outdated Show resolved Hide resolved
@ebolyen ebolyen assigned Oddant1 and unassigned ebolyen Jun 13, 2019
@ebolyen ebolyen moved this from In Review to In Development in 2019.7 Jun 13, 2019
@ebolyen ebolyen moved this from In Development to In Review in 2019.7 Jun 15, 2019
@ebolyen ebolyen assigned ebolyen and unassigned Oddant1 Jun 15, 2019
Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small updates and this is ready I think! Thanks @Oddant1!

Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@ebolyen ebolyen merged commit 35561ff into qiime2:master Jun 19, 2019
2019.7 automation moved this from In Review to Changelog Needed Jun 19, 2019
@thermokarst thermokarst moved this from Changelog Needed to Completed in 2019.7 Jul 10, 2019
@thermokarst
Copy link
Contributor

Hey @Oddant1, can you take a look at this forum post: https://forum.qiime2.org/t/training-feature-classifier-2019-10-error/13025/

It looks like some of the new validation you implemented in this PR is being hit by these users, although it looks like maybe they don't have quite enough information in the error message to fix anything. Also, it looks like maybe the validation isn't quite working as expected? I'm not sure, I would need to take a closer look at this code to fully understand, but wanted to ping you first to see if you have any ideas. Thanks!

@thermokarst thermokarst added this to In Review in 2020.2 via automation Jan 3, 2020
@thermokarst thermokarst moved this from In Review to In Analysis in 2020.2 Jan 3, 2020
@Oddant1
Copy link
Member Author

Oddant1 commented Jan 4, 2020

@thermokarst, apologies for the late reply, I have a theory I will be posting on the forum. I'm not 100% sure it's correct as I was strangely unable to exactly replicate the errors either user experienced, but I got something similar. Also yes I think it's a good idea to make it so the error also prints what character is erroneous and ideally also where said character is within the line.

@thermokarst thermokarst moved this from In Analysis to In Progress - Supplanted Issues in 2020.2 Jan 29, 2020
@Oddant1
Copy link
Member Author

Oddant1 commented Feb 7, 2020

Apologies for forgetting to update this asap. Based on the responses of the forum users in the forum thread, I believe my theory was correct, and they had trailing whitespace causing validation errors. The user then ran into what (I think) was a largely unrelated issue that @nbokulich helped them with.

@Oddant1 Oddant1 removed this from In Progress - Supplanted Issues in 2020.2 Feb 19, 2020
@Oddant1 Oddant1 deleted the DNAFASTAFormat_validator branch June 25, 2020 20:45
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
2019.7
  
Completed
Development

Successfully merging this pull request may close these issues.

DNAFASTAFormat validator is not catching invalid DNA characters
3 participants