finish for coupled model, clean up the tests

src/reconstruction/CoreModel.jl

@@ -358,7 +358,7 @@ end
 end
 
 @_remove_fn reaction CoreModel Int inplace plural begin
-    mask = .! in.(1:n_reactions(model), Ref(reaction_idxs))
+    mask = .!in.(1:n_reactions(model), Ref(reaction_idxs))
     model.S = model.S[:, mask]
     model.c = model.c[mask]
     model.xl = model.xl[mask]
@@ -373,12 +373,7 @@ end
 
 @_remove_fn reaction CoreModel Int plural begin
     n = copy(model)
-    n.S = copy(n.S)
-    n.c = copy(n.c)
-    n.xl = copy(n.xl)
-    n.xu = copy(n.xu)
-    n.rxns = copy(n.rxns)
-    remove_reactions!(model, reaction_idxs)
+    remove_reactions!(n, reaction_idxs)
     return n
 end
 
@@ -394,7 +389,7 @@ end
     remove_reactions(model, [reaction_id])
 end
 
-@_remove_fn reaction CoreModel String begin
+@_remove_fn reaction CoreModel String plural begin
     remove_reactions(model, Int.(indexin(reaction_ids, reactions(model))))
 end
 
@@ -403,10 +398,14 @@ end
 end
 
 @_remove_fn metabolite CoreModel Int plural inplace begin
-    remove_reactions!(model,
-        [ridx for ridx in 1:n_reactions(model) if
-            any(in.(findnz(model.S[:,ridx])[2], Ref(metabolite_idxs)))])
-    mask = .! in.(1:n_metabolites(model), Ref(metabolite_idxs))
+    remove_reactions!(
+        model,
+        [
+            ridx for ridx in 1:n_reactions(model) if
+            any(in.(findnz(model.S[:, ridx])[2], Ref(metabolite_idxs)))
+        ],
+    )
+    mask = .!in.(1:n_metabolites(model), Ref(metabolite_idxs))
     model.S = model.S[mask, :]
     model.b = model.b[mask]
     model.mets = model.mets[mask]
@@ -419,7 +418,7 @@ end
 
 @_remove_fn metabolite CoreModel Int plural begin
     n = deepcopy(model) #everything gets changed anyway
-    remove_metabolites!(model, metabolite_idxs)
+    remove_metabolites!(n, metabolite_idxs)
     return n
 end
 
@@ -435,6 +434,6 @@ end
     remove_metabolites(model, [metabolite_id])
 end
 
-@_remove_fn metabolite CoreModel String begin
+@_remove_fn metabolite CoreModel String plural begin
     remove_metabolites(model, Int.(indexin(metabolite_ids, metabolites(model))))
 end

src/reconstruction/CoreModelCoupled.jl

@@ -164,52 +164,6 @@ function add_reactions(
     )
 end
 
-"""
-    remove_reactions(m::CoreModelCoupled, rxns::Vector{Int})
-
-Remove reaction(s) from a `CoreModelCoupled`.
-
-Also removes any metabolites not involved in any reaction after the deletion.
-
-"""
-function remove_reactions(m::CoreModelCoupled, rxns::Vector{Int})
-    return CoreModelCoupled(
-        remove_reactions(m.lm, rxns),
-        m.C[:, filter(!in(rxns), 1:n_reactions(m))],
-        m.cl,
-        m.cu,
-    )
-end
-
-"""
-    remove_reactions(m::CoreModelCoupled, rxn::Int)
-
-"""
-function remove_reactions(m::CoreModelCoupled, rxn::Int)
-    return remove_reactions(m, [rxn])
-end
-
-"""
-    remove_reactions(m::CoreModelCoupled, rxn::String)
-
-"""
-function remove_reactions(m::CoreModelCoupled, rxn::String)
-    return remove_reactions(m, [rxn])
-end
-
-"""
-    remove_reactions(m::CoreModelCoupled, rxns::Vector{String})
-
-"""
-function remove_reactions(m::CoreModelCoupled, rxns::Vector{String})
-    rxn_indices = [findfirst(isequal(rid), m.lm.rxns) for rid in intersect(rxns, m.lm.rxns)]
-    if isempty(rxn_indices)
-        return m
-    else
-        return remove_reactions(m, rxn_indices)
-    end
-end
-
 """
 Add constraints of the following form to a CoreModelCoupled and return a modified one.
 
@@ -400,3 +354,78 @@ end
     n.lm = change_bounds(model.lm, rxn_ids, lower = lower, upper = upper)
     n
 end
+
+@_remove_fn reaction CoreModelCoupled Int inplace begin
+    remove_reactions!(model, [reaction_idx])
+end
+
+@_remove_fn reaction CoreModelCoupled Int inplace plural begin
+    orig_rxns = reactions(model.lm)
+    remove_reactions!(model.lm, reaction_idxs)
+    model.C = model.C[:, in.(orig_rxns, Ref(Set(reactions(model.lm))))]
+    return nothing
+end
+
+@_remove_fn reaction CoreModelCoupled Int begin
+    remove_reactions(model, [reaction_idx])
+end
+
+@_remove_fn reaction CoreModelCoupled Int plural begin
+    n = copy(model)
+    n.lm = remove_reactions(n.lm, reaction_idxs)
+    n.C = n.C[:, in.(reactions(model.lm), Ref(Set(reactions(n.lm))))]
+    return n
+end
+
+@_remove_fn reaction CoreModelCoupled String inplace begin
+    remove_reactions!(model, [reaction_id])
+end
+
+@_remove_fn reaction CoreModelCoupled String inplace plural begin
+    remove_reactions!(model, Int.(indexin(reaction_ids, reactions(model))))
+end
+
+@_remove_fn reaction CoreModelCoupled String begin
+    remove_reactions(model, [reaction_id])
+end
+
+@_remove_fn reaction CoreModelCoupled String plural begin
+    remove_reactions(model, Int.(indexin(reaction_ids, reactions(model))))
+end
+
+@_remove_fn metabolite CoreModelCoupled Int inplace begin
+    remove_metabolites!(model, [metabolite_idx])
+end
+
+@_remove_fn metabolite CoreModelCoupled Int plural inplace begin
+    orig_rxns = reactions(model.lm)
+    model.lm = remove_metabolites(model.lm, metabolite_idxs)
+    model.C = model.C[:, in.(orig_rxns, Ref(Set(reactions(model.lm))))]
+    return nothing
+end
+
+@_remove_fn metabolite CoreModelCoupled Int begin
+    remove_metabolites(model, [metabolite_idx])
+end
+
+@_remove_fn metabolite CoreModelCoupled Int plural begin
+    n = deepcopy(model) #almost everything gets changed anyway
+    remove_metabolites!(n, metabolite_idxs)
+    return n
+end
+
+@_remove_fn metabolite CoreModelCoupled String inplace begin
+    remove_metabolites!(model, [metabolite_id])
+end
+
+@_remove_fn metabolite CoreModelCoupled String inplace plural begin
+    remove_metabolites!(model, Int.(indexin(metabolite_ids, metabolites(model))))
+end
+
+@_remove_fn metabolite CoreModelCoupled String begin
+    remove_metabolites(model, [metabolite_id])
+end
+
+@_remove_fn metabolite CoreModelCoupled String plural begin
+    remove_metabolites(model, Int.(indexin(metabolite_ids, metabolites(model))))
+end

src/reconstruction/modifications/generic.jl

@@ -20,8 +20,7 @@ with_changed_bounds(args...; kwargs...) = m -> change_bounds(m, args...; kwargs.
 Specifies a model variant without a certain metabolite. Forwards arguments to
 [`remove_metabolite`](@ref). Intended to be used with [`screen`](@ref).
 """
-with_removed_metabolite(args...; kwargs...) =
-    m -> remove_metabolite(m, args...; kwargs...)
+with_removed_metabolite(args...; kwargs...) = m -> remove_metabolite(m, args...; kwargs...)
 
 """
     with_removed_metabolites(args...; kwargs...)

test/reconstruction/CoreModel.jl

@@ -43,7 +43,6 @@ end
 
 @testset "Verify consistency" begin
     cp = test_LP()
-    @test size(cp.S) == (4, 3)
     (new_reactions, new_mets) = verify_consistency(
         cp,
         reshape(cp.S[:, end], :, 1),
@@ -77,7 +76,6 @@ end
 
 @testset "Add reactions (checking existence and consistency)" begin
     cp = test_LP()
-    @test size(cp.S) == (4, 3)
     (new_cp, new_reactions, new_mets) = add_reactions(
         cp,
         cp.S[:, end],
@@ -106,7 +104,6 @@ end
 
 @testset "Add reactions" begin
     cp = test_LP()
-    @test size(cp.S) == (4, 3)
     cp = add_reactions(cp, 2.0 * ones(4), 3 .* ones(4), 2.0, -1.0, 1.0)
     @test size(cp.S) == (8, 4)
     cp = add_reactions(cp, 2.0 * ones(4, 1), 3 .* ones(4), 2 .* ones(1), -ones(1), ones(1))
@@ -184,20 +181,16 @@ end
 
 @testset "Remove reactions" begin
     cp = test_LP()
-    @test size(cp.S) == (4, 3)
-    cp = remove_reactions(cp, 2)
-    @test size(cp.S) == (0, 2)
+    cp = remove_reaction(cp, 2)
+    @test size(cp.S) == (4, 2)
     cp = remove_reactions(cp, [2, 1])
-    @test size(cp.S) == (0, 0)
+    @test size(cp.S) == (4, 0)
 
     cp = test_LP()
-    @test size(cp.S) == (4, 3)
-    cp = remove_reactions(cp, "r0")
-    @test size(cp.S) == (4, 3)
-    cp = remove_reactions(cp, "r1")
-    @test size(cp.S) == (0, 2)
+    cp = remove_reaction(cp, "r1")
+    @test size(cp.S) == (4, 2)
     cp = remove_reactions(cp, ["r2"])
-    @test size(cp.S) == (0, 1)
+    @test size(cp.S) == (4, 1)
 
     lp = CoreModel(
         [1.0 1 1 0; 1 1 1 0; 1 1 1 0; 0 0 0 1],
@@ -210,27 +203,27 @@ end
     )
 
     modLp = remove_reactions(lp, [4; 1])
-    @test stoichiometry(modLp) == stoichiometry(lp)[1:3, 2:3]
-    @test balance(modLp) == balance(lp)[1:3]
+    @test stoichiometry(modLp) == stoichiometry(lp)[:, 2:3]
+    @test balance(modLp) == balance(lp)
     @test objective(modLp) == objective(lp)[2:3]
     @test bounds(modLp)[1] == bounds(lp)[1][2:3]
     @test bounds(modLp)[2] == bounds(lp)[2][2:3]
     @test reactions(modLp) == reactions(lp)[2:3]
-    @test metabolites(modLp) == metabolites(lp)[1:3]
+    @test metabolites(modLp) == metabolites(lp)
 end
 
 @testset "Remove metabolites" begin
     model = load_model(CoreModel, model_paths["e_coli_core.json"])
 
     m1 = remove_metabolites(model, ["glc__D_e", "for_c"])
-    m2 = remove_metabolites(model, "glc__D_e")
-    m3 = remove_metabolites(model, indexin(["glc__D_e", "for_c"], metabolites(model)))
-    m4 = remove_metabolites(model, first(indexin(["glc__D_e"], metabolites(model))))
-
-    @test size(stoichiometry(m1)) == (70, 94)
-    @test size(stoichiometry(m2)) == (71, 94)
-    @test size(stoichiometry(m3)) == (70, 94)
-    @test size(stoichiometry(m4)) == (71, 94)
+    m2 = remove_metabolite(model, "glc__D_e")
+    m3 = remove_metabolites(model, Int.(indexin(["glc__D_e", "for_c"], metabolites(model))))
+    m4 = remove_metabolite(model, first(indexin(["glc__D_e"], metabolites(model))))
+
+    @test size(stoichiometry(m1)) == (70, 22)
+    @test size(stoichiometry(m2)) == (71, 22)
+    @test size(stoichiometry(m3)) == (70, 22)
+    @test size(stoichiometry(m4)) == (71, 22)
     @test all((!in(metabolites(m1))).(["glc__D_e", "for_c"]))
     @test !(["glc__D_e"] in metabolites(m2))
     @test all((!in(metabolites(m3))).(["glc__D_e", "for_c"]))

test/reconstruction/CoreModelCoupled.jl

@@ -119,34 +119,31 @@ end
     cp = convert(CoreModelCoupled, test_LP())
     cp = add_coupling_constraints(cp, 1.0 .* collect(1:n_reactions(cp)), -1.0, 1.0)
 
-    new_cp = remove_reactions(cp, [3; 2])
+    new_cp = remove_reactions(cp, [3, 2])
     @test new_cp isa CoreModelCoupled
-    @test new_cp.C[:] == cp.C[:, 1] # because cp.C[:, 1] comes out as a Vector
+    @test new_cp.C[:] == cp.C[:, 1]
     @test new_cp.cl == cp.cl
     @test new_cp.cu == cp.cu
 
-    new_cp = remove_reactions(cp, 2)
-    @test new_cp.C == cp.C[:, [1; 3]]
+    new_cp = remove_reaction(cp, 2)
+    @test new_cp.C == cp.C[:, [1, 3]]
     @test new_cp.cl == cp.cl
     @test new_cp.cu == cp.cu
 
-    new_cp = remove_reactions(cp, "r1")
+    new_cp = remove_reaction(cp, "r1")
     @test new_cp.C == cp.C[:, 2:3]
     @test new_cp.cl == cp.cl
     @test new_cp.cu == cp.cu
 
-    new_cp = remove_reactions(cp, ["r1"; "r3"])
+    new_cp = remove_reactions(cp, ["r1", "r3"])
     @test new_cp.C[:] == cp.C[:, 2]
     @test new_cp.cl == cp.cl
     @test new_cp.cu == cp.cu
 
-    new_cp = remove_reactions(cp, [1; 4])
+    new_cp = remove_reactions(cp, [1, 4])
     @test new_cp.C == cp.C[:, 2:3]
 
-    new_cp = remove_reactions(cp, "r4")
-    @test new_cp.C == cp.C
-
-    new_cp = remove_reactions(cp, [1; 1; 2])
+    new_cp = remove_reactions(cp, [1, 1, 2])
     @test new_cp.C[:] == cp.C[:, 3]
 end