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 Annotation parsing issue #8

Merged
merged 3 commits into from
Mar 2, 2024
Merged

Conversation

josePereiro
Copy link
Contributor

@josePereiro josePereiro commented Feb 11, 2024

Porting from COBREXA 1.x PR.

Most of the return-default issues were already implemented. Just "generalizing" the annotation parser fixes my issue.

Here are the tests I'm running locally. I'm using the same models from the package test suite, but I'm adding the network I'm interested in (elife-36842-supp10-v2.json). I don't really know if the network's JSON complies with the expected interface, that's why I'm comparing it with the other models.

## - - - - - - - - - - - - - - - - - - - - - - - - 
@time begin
    using JSONFBCModels
    using JSONFBCModels: JSONFBCModel
    using JSONFBCModels: A
    using Test
end

## - - - - - - - - - - - - - - - - - - - - - - - - 
@testset "Type tests" begin
    for name in [
            "iJO1366", 
            "e_coli_core", 
            "elife-36842-supp10-v2", 
            "iML1515"
        ]
        @show name
        path = joinpath(@__DIR__, "test-models", string(name, ".json"))
        model = A.load(path);

        _rxns = A.reactions(model)
        @test _rxns isa Vector{String}
        @test A.reaction_annotations.([model], _rxns) isa Vector{A.Annotations}
        @test A.reaction_gene_association_dnf.([model], _rxns) isa Vector{Union{Nothing, Vector{Vector{String}}}}
        @test A.reaction_name.([model], _rxns) isa Vector{String}
        @test A.reaction_notes.([model], _rxns) isa Vector{A.Notes}
        @test A.reaction_stoichiometry.([model], _rxns) isa Vector{Dict{String, Float64}}

        _mets = A.metabolites(model)
        @test _mets isa Vector{String}
        @test A.metabolite_annotations.([model], _mets) isa Vector{A.Annotations}
        @test A.metabolite_charge.([model], _mets) isa Vector{Int64}
        @test A.metabolite_compartment.([model], _mets) isa Vector{String}
        @test A.metabolite_formula.([model], _mets) isa Vector{Dict{String, Int64}}
        @test A.metabolite_name.([model], _mets) isa Vector{String}
        @test A.metabolite_notes.([model], _mets) isa Vector{A.Notes}

        _genes = A.genes(model)
        @test _genes isa Vector{String}
        @test A.gene_annotations.([model], _genes) isa Vector{A.Annotations}
        @test A.gene_name.([model], _genes) isa Vector{String}
        @test A.gene_notes.([model], _genes) isa Vector{A.Notes}

        @test A.stoichiometry(model) isa AbstractMatrix{Float64}

        # AbstractFBCModels Tests
        A.run_fbcmodel_file_tests(JSONFBCModel, path; name, test_save = true)
    end
end

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4b932b4) to head (d7e5891).

Additional details and impacted files
@@            Coverage Diff            @@
##            master        #8   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          196       197    +1     
=========================================
+ Hits           196       197    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@exaexa
Copy link
Member

exaexa commented Feb 12, 2024

Hi! looks all good, this looks more like the structured notes problem rather than actual annotations (the json has none of these, right?). I'll try to add a proper test so that this doesn't get accidentally broken again. (also we had a semi-formal decision here to just keep the tests on 💯% coverage because software quality :D )

For completeness, is it possible to somehow automatically download that linked json from eLife (or any other kind of repo) ?

src/utils.jl Outdated Show resolved Hide resolved
Co-authored-by: Mirek Kratochvil <[email protected]>
@josePereiro
Copy link
Contributor Author

Hi, the json has no annotations but has notes that are structured. I don't know if this is the expected 'legal' format.

Here an example of both a metabolite and a reaction section.

{
    "id":"1ag3p_c",
    "name":"1-acyl-glycerol 3-phosphate",
    "compartment":"c",
    "charge":-2,
    "formula":"C4H6O7PR",
    "notes":{
        "InChI Key":{
            "notes":"(no InChI key)"
        },
        "KEGG Compound":{
            "id":"C00681",
            "link":"http://identifiers.org/kegg.compound/C00681"
        }
    }
},
{
    "id":"CYTK1",
    "name":"CYTK1",
    "metabolites":{
        "adp_c":1,
        "atp_c":-1,
        "cdp_c":1,
        "cmp_c":-1
    },
    "lower_bound":-1000,
    "upper_bound":1000,
    "gene_reaction_rule":"MMSYN1_0347",
    "subsystem":"Nucleotide metabolism",
    "notes":{
        "EC number":{
          "id":"2.7.4.25",
          "link":"http://identifiers.org/ec-code/2.7.4.25"
        }
    }
}

Regarding downloading, below are the links to access the Additional Files associated with the paper, as well as the specific link to download file 10 (the network). The current issue (i think) with the JSONFBCModels workflow is that the link retrieves a .zip file.

@exaexa
Copy link
Member

exaexa commented Mar 2, 2024

@josePereiro apologies for the delay, parental leave problems :D

Hi, the json has no annotations but has notes that are structured. I don't know if this is the expected 'legal' format.

There's absolutely no standard for the notes formatting (as generally for the JSON model format). AFAIK cobrapy just goes for "whatever fits into the JSON". Quite conveniently (not), SBML has notes in "free form XML", and cobratoolbox has notes in free form matlab... In turn, hard-converting to string and then just trying to convert back is literally the only portable way that we can ever do.

Will try to get this merged ASAP.

@exaexa
Copy link
Member

exaexa commented Mar 2, 2024

interesting, for whatever reason I'm getting full coverage on my computer while codecov keeps whining on missing return line coverage (for no apparent reason). Let's dive deeper here. 🤣

Copy link
Member

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

ok all good now. Thanks a lot!

@exaexa exaexa merged commit d0cfd5d into COBREXA:master Mar 2, 2024
2 checks passed
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