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

refactor: standard-GEM #257

Merged
merged 9 commits into from
Jun 29, 2021
Merged

refactor: standard-GEM #257

merged 9 commits into from
Jun 29, 2021

Conversation

edkerk
Copy link
Member

@edkerk edkerk commented May 14, 2021

Main improvements in this PR:

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Selected devel as a target branch (top left drop-down menu)
  • If needed, asked first in the Gitter chat room about this PR

Copy link
Collaborator

@feiranl feiranl left a comment

Choose a reason for hiding this comment

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

Everything looks good, except there is deletion of one entry of model xml file, if that is ok, then everything is fine.

Copy link
Member

@mihai-sysbio mihai-sysbio left a comment

Choose a reason for hiding this comment

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

Awesome!

Looking at the /model/* files, I see they are named yeastGEM instead of the expected yeast-GEM. Any thoughts on the standard-GEM indication that

All model files must be named the same as the repository

?

@edkerk
Copy link
Member Author

edkerk commented Jun 24, 2021

Awesome!

Looking at the /model/* files, I see they are named yeastGEM instead of the expected yeast-GEM. Any thoughts on the standard-GEM indication that

All model files must be named the same as the repository

?

I don't see a reason why not to adhere to the standard-GEM indication on this, so seems fine to correct this.

@edkerk
Copy link
Member Author

edkerk commented Jun 24, 2021

Everything looks good, except there is deletion of one entry of model xml file, if that is ok, then everything is fine.

@feiranl This is an artifact of the empty subSystems. I've now manually removed them (model=rmfield(model,'subSystems')) before running saveYeastModel. This will be fully fixed when subSystems are added (see #11).

edkerk added 2 commits June 24, 2021 11:59
This partially reverts commit 919d396.
Restores the model.id so it will not have the dash (so yeastGEM instead of yeast-GEM)
@mihai-sysbio
Copy link
Member

@edkerk I'm curious, why was 0ef8f3f necessary?

edkerk added 2 commits June 29, 2021 13:51
mistakingly reverted in last commit
Benjamin's email no longer active, replace with Eduard's
@edkerk
Copy link
Member Author

edkerk commented Jun 29, 2021

Just for reference, coming back to naming of the model files. In commit 919d396, I also changed the model.id field from yeastGEM to yeast-GEM. However, - is not allowed in this ID in SBML, so that it either would end up with an invalid SBML file, or it would end up with a model.id that reads yeast__45__GEM if replaced with ASCII code as COBRA routinely does. This also raises issues when the model is then loaded with RAVEN or cobrapy instead as these do not automatically modify the IDs to replace ASCII codes. So best to prevent all of this, while standard-GEM also does not specify that the model.id should have a certain format, just the model filename. This is now fixed with 0ef8f3f and aab4e8d.

@edkerk edkerk requested a review from mihai-sysbio June 29, 2021 12:25
Copy link
Member

@mihai-sysbio mihai-sysbio left a comment

Choose a reason for hiding this comment

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

Interesting observation with the - in SBML. Perhaps it would be nice to add some automated validation of the SBML as a GH Action at some point in the future.

Anyway, everything looks good to me!

@edkerk
Copy link
Member Author

edkerk commented Jun 29, 2021

@mihai-sysbio Very good point, the SBML documentation is very clear on what is allowed, so it would just involve a few regex-checks.

@edkerk edkerk merged commit 6b3235f into devel Jun 29, 2021
@edkerk edkerk deleted the refactor/standard-GEM branch June 29, 2021 12:32
This was referenced Jul 1, 2021
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.

3 participants