-
Notifications
You must be signed in to change notification settings - Fork 14
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
New source: BiGG Models Metabolite Database #124
base: main
Are you sure you want to change the base?
Conversation
…eded from ensure_df()
Hi Charlie, I just added new changes. Please review them in your free time. Thanks! |
version=version, | ||
) | ||
|
||
for v in bigg_df.values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v
is a bad variable name. Better is to use tuple unpacking
for v in bigg_df.values: | |
for bigg_id, name in bigg_df.values: |
src/pyobo/sources/bigg.py
Outdated
skiprows=18, | ||
header=None, | ||
names=HEADER, | ||
usecols=['bigg_id', 'name'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the other columns? database_links
has lots of information worth parsing out, for example. old_bigg_ids
can be put in the alternative identifier field in the pyobo.Term
) | ||
|
||
|
||
def get_terms(force: bool = False, version: Optional[str] = None) -> Iterable[Term]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check CI - this needs a docstring
@@ -0,0 +1,63 @@ | |||
# -*- coding: utf-8 -*- | |||
|
|||
"""Converter for bigg.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix stylization of BiGG
|
||
|
||
def get_obo(force: bool = False) -> Obo: | ||
"""Get bigg as OBO.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylization
def get_obo(force: bool = False) -> Obo: | ||
"""Get bigg as OBO.""" | ||
version = bioversions.get_version("bigg") | ||
# version = '1.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove dead code
# version = '1.2' | ||
return Obo( | ||
ontology=PREFIX, | ||
name="bigg models metabolites database", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title case + stylization
name="bigg models metabolites database", | ||
iter_terms=get_terms, | ||
iter_terms_kwargs=dict(force=force, version=version), | ||
typedefs=[has_role], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this included?
src/pyobo/sources/bigg.py
Outdated
for v in bigg_df.values: | ||
bigg_id = v[0] | ||
name = v[1] | ||
synonyms = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why include this blank list?
@cthoyt This is still a work in progress PR.
get_obo
function. Could you please test it on your end and see where I am going wrong?Thanks!