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

importYaml is not compatible with MATLAB 2022b #614

Closed
feiranl opened this issue Jun 7, 2023 · 5 comments
Closed

importYaml is not compatible with MATLAB 2022b #614

feiranl opened this issue Jun 7, 2023 · 5 comments
Assignees
Labels

Comments

@feiranl
Copy link
Collaborator

feiranl commented Jun 7, 2023

Current behavior:

Reproducing these results:

  1. In this environment IOS system, MATLAB2022b with Human-GEM main branch and RAVEN 2.8.1
  2. Run model = importYaml('Human-GEM.yml');
  3. See error:
Error using readlines
Argument must be a text scalar.

Error in importYaml (line 38)
    line_raw=readlines(yamlFilename');

I checked the yamlFilename = 'Human-GEM.yml'
yamlFilename' =

13×1 char array

'H'
'u'
'm'
'a'
'n'
'-'
'G'
'E'
'M'
'.'
'y'
'm'
'l'

After removing the transpose sign, the code works fine.

I double checked the corresponding code readYAMLmodel in RAVEN using
model = readYAMLmodel('Human-GEM.yml'); which works fine, and the corresponding line is line_raw=readlines(fileName); which doesn't contain the transpose sign.

According to the blame file, @edkerk coded for this line in Human-GEM before. Hi Ed, do you have any suggestion for this one? 9aea8ae

@feiranl feiranl added the bug label Jun 7, 2023
@edkerk edkerk self-assigned this Jun 7, 2023
@edkerk
Copy link
Member

edkerk commented Jun 7, 2023

This (and a somewhat related bug) are now fixed here: d8c8378.

As you refer to RAVEN's readYAMLmodel, the above fixes were already resolved there. This also rises a bigger issue: should readYAMLmodel be used instead of importYaml?

These two functions generate the same model structure in MATLAB, with the following exceptions:

  1. Order of fields in the MATLAB structure is a little bit different (this has no further significance).
  2. readYAMLmodel supports metNotes field, and GECKO3 ecModels (at the moment not relevant for Human-GEM repo though)
  3. readYAMLmodel will always show the name, and date fields, even if empty.
  4. readYAMLmodel does not print output to command window to show its progress (this is also not that essential anymore. It was good when the function was slow (multiple minutes), but it now takes ~10 seconds).
  5. metCharges: importYaml gives it as "in64", while readYAMLmodel gives "double". According to the definitions of model structures (RAVEN, but even COBRA), it should be double.

IMHO, readYAMLmodel can therefore replace importYaml. However, that introduces a software dependency (RAVEN) for model development (as the yml model is the only model format there). I don't think that this is a problem though, as the model structure that is loaded is in RAVEN format anyway, implying that development (primarily?) takes place with RAVEN. But this has not been formalized anyway, as far as I can see from the guidlines and wiki.

@haowang-bioinfo
Copy link
Member

This (and a somewhat related bug) are now fixed here: d8c8378.

good to know this

This also rises a bigger issue: should readYAMLmodel be used instead of importYaml?

better keep it simple for now, instead of a bigger issue

@feiranl
Copy link
Collaborator Author

feiranl commented Jun 7, 2023

Thanks for the quick response!

I also vote for having a model read function in this repo to keep it simple for now.

@edkerk
Copy link
Member

edkerk commented Jun 7, 2023

Fair enough, I'll then make a PR and so that this issue can be solved.

@edkerk edkerk mentioned this issue Jun 7, 2023
3 tasks
@feiranl
Copy link
Collaborator Author

feiranl commented Jun 8, 2023

Fixed in #619

@feiranl feiranl closed this as completed Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants