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.

Sign up for GitHub

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

Jump to bottom

feat: add embl gems repository #1282

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from
Open

feat: add embl gems repository #1282

wants to merge 6 commits into from

Conversation

achillesrasquinha
Copy link
Contributor

Midnighter
Midnighter requested changes Sep 24, 2022
View reviewed changes
Copy link
Member

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

A couple of smaller changes are needed, otherwise this looks great. Thanks for the contribution!

src/cobra/io/web/bigg_models_repository.py
@@ -1,4 +1,4 @@
"""Provide a concrete implementation of the BioModels repository interface."""
"""Provide a concrete implementation of the BiGG repository interface."""
Copy link
Member

Choose a reason for hiding this comment

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

Thanks 😊

src/cobra/io/web/embl_gems_repository.py
decoded_path = _decode_model_path(model_id)

filename = f"{model_id}.xml.gz"
print(self._url.join(decoded_path).join(filename))
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the print or use logging instead.

tests/test_io/test_web/test_load.py
load_model("BIOMD0000000633", cache=False, repositories=[biomodels])
biomodels.get_sbml.assert_called_once_with(model_id="BIOMD0000000633")

def test_biomodels_access(embl_gems: Mock) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Need a different name here:

Suggested change
def test_biomodels_access(embl_gems: Mock) -> None:
def test_embl_gems_access(embl_gems: Mock) -> None:

tests/test_io/test_web/test_load.py

"""
load_model("Abiotrophia_defectiva_ATCC_49176", cache=False, repositories=[embl_gems])
biomodels.get_sbml.assert_called_once_with(model_id="Abiotrophia_defectiva_ATCC_49176")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
biomodels.get_sbml.assert_called_once_with(model_id="Abiotrophia_defectiva_ATCC_49176")
embl_gems.get_sbml.assert_called_once_with(model_id="Abiotrophia_defectiva_ATCC_49176")

src/cobra/io/web/embl_gems_repository.py
**kwargs,
) -> None:
"""
Initialize a EMBL GEMs repository interface.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Initialize a EMBL GEMs repository interface.
Initialize an EMBL GEMs repository interface.

src/cobra/io/web/embl_gems_repository.py
return f"{alphabet}/{directory}/{model_path}"


class EMBLGems(AbstractModelRepository):
Copy link
Member

Choose a reason for hiding this comment

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

I think EMBLGEMs would also be an acceptable name, although your version is more legible.

src/cobra/io/web/embl_gems_repository.py
from .abstract_model_repository import AbstractModelRepository


def _decode_model_path(model_path):
Copy link
Member

Choose a reason for hiding this comment

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

Make this a class method.

src/cobra/io/web/embl_gems_repository.py

"""
super().__init__(
url="https://github.com/cdanielmachado/embl_gems/blob/master/models/",
Copy link
Member

Choose a reason for hiding this comment

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

Access the raw content directly:

Suggested change
url="https://github.com/cdanielmachado/embl_gems/blob/master/models/",
url="https://raw.githubusercontent.com/cdanielmachado/embl_gems/master/models/",

@cdiener
Copy link
Member

cdiener commented Sep 24, 2022 •
edited
Loading

The naming is confusing I think. Biomodels is the official GEM repository from EMBL by their own specification, so that should rather be called carveME repository or similar. And I would include a link to the publication in the docstring to give some recognition to the author.

Looks great though and will be a helpful addition for sure.

@cdiener cdiener added the stale The issue or pull request lacks activity. label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Midnighter Midnighter Midnighter requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
stale The issue or pull request lacks activity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add EMBL GEMs to Web IO Repository.
3 participants
@achillesrasquinha @cdiener @Midnighter