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

Fix "what SYMBOL" issue #10

Merged
merged 3 commits into from
Mar 3, 2023
Merged

Fix "what SYMBOL" issue #10

merged 3 commits into from
Mar 3, 2023

Conversation

sterrettJD
Copy link
Contributor

parse_ko() has been raising an error for my use (through AMON, consistent with AMON issue #13. When running test_parse_KEGG.py, tests for this function fail.

A typical error message would be ValueError: What is SYMBOL in K00001? from raise ValueError('What is {} in {}?'.format(current_entry_name, ko_dict['ENTRY'])) at line 212.

To circumvent this issue, I added SYMBOL to NOT_CAPTURED_KO_FIELDS. But then I got a similar issue at each of the next steps with the CO fields and PATHWAY fields, so I added NETWORK and INCLUDING to those (respectively). After these changes, all tests pass, and AMON is no longer erroring.

I could be wrong, but I'm guessing these fields have been added to the KEGG reports since KEGG_Parser was last updated? Any ideas on if this is the right approach to fixing this issue?

Copy link
Member

@kthurimella kthurimella left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Not sure where else we can make improvements. Thanks for adding these fields in.


NOT_CAPTURED_PATHWAY_FIELDS = ('GENE', 'ORGANISM', 'REFERENCE', 'AUTHORS', 'TITLE', 'JOURNAL')
NOT_CAPTURED_PATHWAY_FIELDS = ('GENE', 'ORGANISM', 'REFERENCE', 'AUTHORS', 'TITLE', 'JOURNAL', 'INCLUDING')
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense and it looks like it must have been added afterwards. I'm wondering if there is a case where we can capture fields that may get added, but we don't update the not captured fields, just to keep a bit less of a dependency on keeping up with KEGG fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Hopefully we can discuss ways to do this during our next code review! @JTFouquier @bsantan @acolorado1 @albatalavera @jboconnor13 (tagging you all so we can discuss at some point)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't have a chance to specifically discuss a better fix, but did decide to go ahead and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants