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: disable adding prefix 'M_' and 'R_' in exprotModel function #356

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

haowang-bioinfo
Copy link
Member

@haowang-bioinfo haowang-bioinfo commented Aug 9, 2021

Main improvements in this PR:

I hereby confirm that I have:

@edkerk
Copy link
Member

edkerk commented Aug 9, 2021

To my opinion, importModel should then also be changed, see my comment in #353.

@edkerk edkerk added this to the 2.6.0 milestone Aug 9, 2021
@edkerk
Copy link
Member

edkerk commented Aug 9, 2021

Also, I suggest this would be part of a minor release (2.5.2 -> 2.6.0). It might break compatibility of scripts that would have relied on the identifiers having their prefix removed, and full COBRA <-> RAVEN compatibility.

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.

Very nice to have this fix in Raven. I noted a few things

  • in io/exportModel.m:510 this should be updated rdf:about="#meta_R_' model.rxns{i} '"
  • not compulsory: there's a reference to meta_M in doc/io/exportModel.html:391 and to meta_R in the same file on line 569
  • I think @edkerk noted that there is no function that reports all SBML-invalid identifiers, which I agree should be part of this PR that solves refactor: remove prefix 'M_' and 'R_' in exprotModel/importModel functions #353 see below

@mihai-sysbio
Copy link
Member

* [ ]  I think @edkerk noted that there is no function that reports all SBML-invalid identifiers, which I agree should be part of this PR that solves [refactor: remove prefix 'M_' and 'R_' in exprotModel/importModel functions #353](https://github.com/SysBioChalmers/RAVEN/issues/353)

On second thought, @edkerk would it be okay to resolve the concern above as a different PR, to keep PRs small and targeted? This PR will then resolve only a part of #353, which makes sense to me.

Copy link
Member

@edkerk edkerk left a comment

Choose a reason for hiding this comment

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

  • Remove line 225-231, where c_ is appended to model.comps.

@haowang-bioinfo
Copy link
Member Author

there's a reference to meta_M in doc/io/exportModel.html:391 and to meta_R in the same file on line 569

the html files will be corrected by the m2html later

@mihai-sysbio
Copy link
Member

the html files will be corrected by the m2html later

As far as I can see m2html is not meant to be run automatically, so would it make sense to run it now so that all the changes are consistently applied?

@edkerk
Copy link
Member

edkerk commented Aug 12, 2021

We have the routine to run updateDocumentation just before making a new release (example #357), but it doesn't hurt to run it earlier already.

@edkerk edkerk self-requested a review August 12, 2021 18:10
Copy link
Member

@edkerk edkerk left a comment

Choose a reason for hiding this comment

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

Successful I/O of yeast-GEM.

@haowang-bioinfo
Copy link
Member Author

Here I merge this since tiny progress is still progress. The ID check function and importModel will be modified in other PRs.

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