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

feat: SBO terms for reaction bounds #1631

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

BenjaSanchez
Copy link
Contributor

Added SBO terms for all lower/upper bounds, based on the definitions from SBO:

These definitions are consistent with cobrapy's after merge of opencobra/cobrapy#949

Tested here: BenjaSanchez/yeast-GEM@086b1cb

I hereby confirm that I have:

  • Tested my code on my own machine
  • Followed the guidelines in the Contributing Guide
  • Selected develop as a target branch (top left drop-down menu)

@rmtfleming
Copy link
Member

Thanks for this contribution Ben. Please can you add a feature to the associated test function, or create a new one associated with this new feature:
https://github.com/opencobra/cobratoolbox/tree/master/test/verifiedTests/base/testIO

@BenjaSanchez
Copy link
Contributor Author

@rmtfleming I have now added the corresponding test in testWriteSBML.m. Note that as far as I can tell, nowhere in the code is the SBML structure itself (the output from writeSBML.m) tested; you might consider in the future testing all other fields of said structure (compartment, species, reaction, fbc_geneProduct, etc).

@tpfau
Copy link
Contributor

tpfau commented Oct 8, 2020

@BenjaSanchez how would you test the structure? using validateSBML?
We know that IO does not necessarily produce the exact same output model/xml file, but in testWriteSBML we do check whether the model is essentially the same (with the exception of a few fields which are different as commented in the test.
Implicitly this also tests whether the model can be read by libsbml, as otherwise we would not be able to read it back in. But yes, an explicit test would probably be good.

@rmtfleming
Copy link
Member

cobratoolbox/external/base/utilities/cellstructeq/structeq.m might be useful

@BenjaSanchez
Copy link
Contributor Author

BenjaSanchez commented Oct 9, 2020

@tpfau I imagine similar checks to the one I introduced here could be performed to all other fields in the SBML structure (i.e. check they have the proper SBO terms). However, maybe checking that the structure doesn't change + remains SBML-compatible is already enough; I only added the test in this PR after @rmtfleming's request, and noticed that there's not really an equivalent to it in the rest of the test suite, so the test looks a bit out of place at the moment.

@rmtfleming not sure how structeq.m would help here, as testWriteSBML.m already tests if loaded & written models are the same.

@rmtfleming rmtfleming merged commit 942be7d into opencobra:develop Oct 15, 2020
@BenjaSanchez BenjaSanchez deleted the feat/bound-sbo-terms branch October 15, 2020 11:35
BenjaSanchez added a commit to SysBioChalmers/yeast-GEM that referenced this pull request Nov 24, 2020
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