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

Solve convert(StandardModel, model::JSONModel) issue #822

Merged
merged 8 commits into from
Mar 2, 2024

Conversation

josePereiro
Copy link
Contributor

@josePereiro josePereiro commented Feb 11, 2024

Description

Hi, I'm trying to load a json model (download link: elife-36842-supp10-v2.json) from this paper.

The following code throw an error

using COBREXA
model = COBREXA.load_json_model(fn);
convert(StandardModel, model)

The error is related with the management of missing keys in the JSONModel... Mustly notes and annotations...

This pull request aim to solve them by returning default (empty) values when need it and generalizing the parsing mechanism.

Saludos

Checklist

Feel free to open PRs that do not satisfy the whole checklist. On the other
hand, all these are required for merging.

  • All code is formatted with
    JuliaFormatter
    (run using JuliaFormatter; format("path/to/COBREXA.jl");)
  • Tests work (try with ]test COBREXA)
  • All new functions have proper docstrings
  • Documentation builds correctly (try with cd docs; julia make.jl)

@stelmo
Copy link
Collaborator

stelmo commented Feb 11, 2024

Hi there! We are actually in the process of releasing COBREXA v2 (see here). We can definitely take a look at this PR, but you might want to consider porting it over here. We split out the model loading interface from the analysis functions, and introduced a much cleaner framework to make building complex models really easy.

@exaexa
Copy link
Collaborator

exaexa commented Feb 11, 2024

Hi @josePereiro, thanks a lot! I guess the patch looks OK, I'll try to merge this here (and take it apart into the new version's repositories) ASAP.

Among other things the (external) CI is currently a little dead; I'll check the stuff manually later.

For the "formatting" CI, if you click details there should be a patch to apply, but you don't need to care there (I'll do).

@josePereiro
Copy link
Contributor Author

josePereiro commented Feb 11, 2024

Hi, it's good to hear about the progress on COBREXA v2. It will be great to have functionality separated into modules. I would suggest placing a notice in the COBREXA v1 package about this "news"; otherwise, it may seem like the project's development/maintenance status is lower.

I've opened a new PR on JSONFBCModels. Most of the default functionality was already implemented, but the model at hand has deeper nested annotations, so the parser fails...

@exaexa
Copy link
Collaborator

exaexa commented Mar 2, 2024

/format

Copy link
Contributor

github-actions bot commented Mar 2, 2024

❌ Auto-formatting triggered by this comment failed, perhaps someone pushed to the PR in the meantime?

@exaexa
Copy link
Collaborator

exaexa commented Mar 2, 2024

I tested this manually, all is OK. Thanks!

`dflt` somehow triggers `deflate` in my brain instead of `default` :D
@exaexa exaexa changed the base branch from master to develop March 2, 2024 19:31
@exaexa exaexa merged commit 543125c into LCSB-BioCore:develop Mar 2, 2024
1 check passed
@exaexa
Copy link
Collaborator

exaexa commented Mar 2, 2024

@josePereiro I will try to release this asap (not today, too late). If I forget about it PLEASE do feel free to remind me, I want to release this but in case it's not out next wednesday or so I just forgot. :D

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