Added loopless fba #60

marvinvanaalst · 2021-04-03T14:18:07Z

Added loopless FBA

Two interfaces:

loopless, to do the FBA and loopless FBA after each other
loopless_solution to work on an existing FBA solution

It should be straight-forward to replace parts of the functionality, as most of it was pulled out into separate functions. This should make merging it with other developments in the FBA routine easier, since it heavily depends on it

stelmo

Nice, and thanks for adding in tests!

src/analysis/looplessfba.jl

stelmo

See comments :)

stelmo · 2021-04-08T10:23:14Z

src/analysis/loopless_flux_balance_analysis.jl

+
+    # Is there a way to get the objective indices if we set the 
+    # objective via `change_objective`? then we could recycle the flux_balance_analysis
+    # method in a better way


Yes, JumP.objective_function, look at line 37 of parimonious_flux_balance_analysis.jl for a use case.

stelmo · 2021-04-08T10:24:11Z

src/analysis/loopless_flux_balance_analysis.jl

+    objective_func::Union{Reaction,Array{Reaction,1}},
+    weights=[],


Can be wrapped into the modifications? Ideally all the analysis functions would have the same arguments where possible

stelmo · 2021-04-08T10:25:15Z

src/analysis/loopless_flux_balance_analysis.jl

+    objective_indices = _get_reaction_indices(model, objective_func)
+    opt_weights = _get_objective_weights(model, objective_indices, weights)
+    change_objective(objective_func, weights=weights)(model, opt_model)


Not sure this is necessary if you have the callbacks.

stelmo · 2021-04-08T10:28:12Z

src/analysis/loopless_flux_balance_analysis.jl

+function _set_solver_attibutes!(opt_model, solver_attributes)
+    if !isempty(solver_attributes) # set other attributes
+        for (k, val) in solver_attributes
+            set_optimizer_attribute(opt_model, k, val)
+        end
+    end
+end


Already have similar functionality in src/base/utilities.jl as change_solver_attributes.

stelmo · 2021-04-08T10:28:53Z

src/analysis/loopless_flux_balance_analysis.jl

+function _set_additional_constraints!(model, opt_model, constraints)
+    for (rxnid, con) in constraints
+        ind = model.reactions[findfirst(model.reactions, rxnid)]
+        set_bound(ind, opt_model; lb=con[1], ub=con[2])
+    end
+end


See previous comment and change_constraint

stelmo · 2021-04-08T10:29:19Z

src/analysis/loopless_flux_balance_analysis.jl

+function _get_reaction_indices(model, objective_func)
+    if typeof(objective_func) == Reaction
+        objective_indices = [model[objective_func]]
+    else 
+        objective_indices = [model[rxn] for rxn in objective_func]
+    end
+    return objective_indices
+end
+
+function _get_objective_weights(model, objective_indices, weights)
+    if isempty(weights)
+        weights = ones(length(objective_indices))
+    end
+    opt_weights = zeros(length(model.reactions))
+
+    # update the objective function tracker
+    wcounter = 1
+    for i in eachindex(model.reactions)
+        if i in objective_indices
+            opt_weights[i] = weights[wcounter]
+            wcounter += 1
+        end
+    end
+    return opt_weights
+end


Could probably read this out using JuMP, see comment below.

stelmo · 2021-04-08T10:36:02Z

src/analysis/loopless_flux_balance_analysis.jl

+function _constrain_objective_value!(opt_model, opt_weights, objective_indices)
+    λ = objective_value(opt_model)
+    v = opt_model[:x]
+    @constraint(
+        opt_model,
+        λ <= sum(opt_weights[i] * v[i] for i in objective_indices) <= λ
+    )
+end


Not sure if necessary to make this a function. Also look at parsimonious_flux_balance_analysis.jl to see how to make this more robust, specifically lines 51 - 58.

stelmo · 2021-04-08T10:37:17Z

src/analysis/loopless_flux_balance_analysis.jl

+    opt_weights,
+    objective_indices,


hopefully not necessary after comments

stelmo · 2021-04-08T10:37:53Z

src/analysis/loopless_flux_balance_analysis.jl

+    return loopless_solution(
+        model,
+        opt_model,
+        opt_weights,
+        objective_indices,
+        fluxes,
+    )


src/analysis/loopless_flux_balance_analysis.jl

stelmo

Very nice! Just a couple of tiny changes and then it'll be ready :)

stelmo · 2021-04-09T06:41:16Z

src/analysis/loopless_flux_balance_analysis.jl

-    COBREXA.JuMP.termination_status(opt_model) in [MOI.OPTIMAL, MOI.LOCALLY_SOLVED] ||
-        return nothing
-    return Dict(zip(reactions(model), value.(opt_model[:x])))
+function add_loopless(model::M, opt_model, fluxes::Dict{String,Float64})::JuMP.Model where {M <: MetabolicModel}


I think we need COBREXA.JuMP.Model here

stelmo · 2021-04-09T06:46:40Z

src/analysis/loopless_flux_balance_analysis.jl

-"""
-    loopless_solution(args...)::Union{Dict{String,Float64},Nothing}
-
-Convert an existing FBA solution to a loopless one. Removes as many loops as possible usingthe method from [1]
-
-References:
-[1] CycleFreeFlux: efficient removal of thermodynamically infeasible
-    loops from flux distributions. Desouki AA, Jarre F, Gelius-Dietrich
-    G, Lercher MJ. Bioinformatics. 2015 Jul 1;31(13):2159-65. doi:
-    10.1093/bioinformatics/btv096.
-"""


Don't get rid of this!

stelmo · 2021-04-09T06:50:12Z

src/analysis/loopless_flux_balance_analysis.jl

+function constrain_objective_value()
+    return (model, opt_model) -> begin
+        λ = objective_value(opt_model)
+        old_objective = objective_function(opt_model)
+        @constraint(
+            opt_model,
+            λ <= sum(old_objective) <= λ
+        )


Maybe move this to src/utilities.jl since it is now a callback, it could maybe be used in other functions. But give it an optimum_bound argument so that I can directly use it in src/analysis/flux_variability_analysis.jl on line 159 - 166 :)

exaexa · 2021-04-09T09:18:08Z

Ey,

can we separate out the "fixes" of CobraModel to StandardModel in docs/ and merge them separately?

In particular, I'm doing that in parallel now, so I was kinda expected to see the conflict here :]

Otherwise it looks cool, will give it a deeper dive later today

laurentheirendt · 2021-04-12T06:30:49Z

@marvinvanaalst and @exaexa can we wrap this up?

exaexa

👍 from me (if the formatting doesn't trigger formatcheck failure)

EDIT: apparently it doesn't trigger any failure, feel free to merge. I'll eventually move my cleaning monsterpullrequest onto this.

exaexa · 2021-04-12T06:46:21Z

src/analysis/flux_balance_analysis.jl

@@ -97,7 +97,7 @@ function flux_balance_analysis_dict(
    model::M,
    args...;
    kwargs...,
-)::Union{Dict{String,Float64},Nothing} where {M<:MetabolicModel}
+)::Union{Dict{String,Float64},Nothing} where {M <: MetabolicModel}


this might cause a tiny problem later, Formatter formats the typenames without spaces around <: for me. Does this come from the formatter?

exaexa · 2021-04-12T06:47:23Z

src/analysis/loopless_flux_balance_analysis.jl

+    G, Lercher MJ. Bioinformatics. 2015 Jul 1;31(13):2159-65. doi:
+    10.1093/bioinformatics/btv096.
+"""
+function add_loopless_vec(model::M, opt_model, fluxes::Dict{String,Float64})::Union{Array{Float64,1},Nothing} where {M <: MetabolicModel}


this looks nice, I'll see if we can unify the code with the main FBA later.

exaexa · 2021-04-12T06:47:51Z

src/base/utilities.jl

-    top_n = 8,
-    ignorebound = 1000.0,
-    verbose = true,
+    top_n=8,


is this from JuliaFormatter?

Added loopless fba

marvinvanaalst requested a review from stelmo April 3, 2021 14:18

stelmo requested changes Apr 3, 2021

View reviewed changes

src/analysis/looplessfba.jl Outdated Show resolved Hide resolved

laurentheirendt assigned marvinvanaalst Apr 8, 2021

marvinvanaalst force-pushed the mva-loopless-fba branch from 2a98b5a to a90f7cb Compare April 8, 2021 09:29

marvinvanaalst added 4 commits April 8, 2021 11:30

Added loopless fba

442bb88

Fixed loopless fba and docs

fb2883b

Updated gitignore

a0f6500

Updated test with add_reactions macro

1cf4854

marvinvanaalst force-pushed the mva-loopless-fba branch from a90f7cb to 1cf4854 Compare April 8, 2021 09:35

laurentheirendt requested review from exaexa and stelmo April 8, 2021 09:56

stelmo requested changes Apr 8, 2021

View reviewed changes

stelmo mentioned this pull request Apr 8, 2021

Add references to methods where possible #76

Closed

Refactored loopless to match callback scheme

7b817c3

stelmo requested changes Apr 9, 2021

View reviewed changes

marvinvanaalst added 2 commits April 9, 2021 09:02

Added error handling

8d75b77

Fixed loopless FBA dict and re-added docstrings

1a0cb56

stelmo approved these changes Apr 9, 2021

View reviewed changes

exaexa approved these changes Apr 12, 2021

View reviewed changes

laurentheirendt merged commit 573b868 into master Apr 12, 2021

laurentheirendt deleted the mva-loopless-fba branch April 12, 2021 07:13

exaexa pushed a commit that referenced this pull request Jul 8, 2021

Merge pull request #60 from LCSB-BioCore/mva-loopless-fba

3db6eb6

Added loopless fba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added loopless fba #60

Added loopless fba #60

marvinvanaalst commented Apr 3, 2021

stelmo left a comment

stelmo left a comment

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo Apr 8, 2021

stelmo left a comment

stelmo Apr 9, 2021

stelmo Apr 9, 2021

stelmo Apr 9, 2021

exaexa commented Apr 9, 2021

laurentheirendt commented Apr 12, 2021

exaexa left a comment •

edited

exaexa Apr 12, 2021

exaexa Apr 12, 2021

exaexa Apr 12, 2021

		objective_func::Union{Reaction,Array{Reaction,1}},
		weights=[],

Added loopless fba #60

Added loopless fba #60

Conversation

marvinvanaalst commented Apr 3, 2021

Added loopless FBA

stelmo left a comment

Choose a reason for hiding this comment

stelmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stelmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exaexa commented Apr 9, 2021

laurentheirendt commented Apr 12, 2021

exaexa left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exaexa left a comment •

edited