Improve MMDF #5
Improve MMDF #5
Conversation
stelmo
force-pushed
the
sew-mmdf-improvements
branch
from
d99b157 to
63d852d
Compare
|
Something is going wrong with the docs + not sure how to make the CI ignore the plots (CairoMakie should not be a test dep) |
There was a problem hiding this comment.
Can you separate out the actual mmdf fixes (esp. the argument organization, mergeable immediately IMO) from the code moving (that shadows any other fixes in the code- if possible always split code moves from actual edits, also that should be rebased on #2 because it does similar thing differently) and from the makie in docs (I've got a feeling that we won't be able to get that working anytime soon because Makie became quite a monster)
Also what changed in the code that the mmdf is so different?
Also also given the MMDF is negative doesn't that mean that the model is in fact thermodynamically infeasible?
just |
| @test isapprox(mmdf_solution.min_driving_force, -2.4739129, atol = TEST_TOLERANCE) #src | ||
|
|
||
| # ## Plot the results | ||
| # We can see that the ΔG bottleneck is 2.5 kJ/mol, and that there is a |
| @@ -197,6 +185,7 @@ function max_min_driving_force_analysis( | |||
| optimized_values( | |||
| constraints; | |||
| objective = constraints.min_driving_force.value, | |||
| sense = Minimal, | |||
There was a problem hiding this comment.
the problem is literally called maximization no?
|
|
||
| reaction_subset = | ||
| isempty(reference_flux) ? A.reactions(model) : | ||
| collect(k for (k, v) in reference_flux if abs(v) > reference_flux_atol) |
There was a problem hiding this comment.
filter(), also omitting zero fluxes contradicts the mmdf paper no?
| =# | ||
|
|
||
| reaction_subset = | ||
| isempty(reference_flux) ? A.reactions(model) : |
There was a problem hiding this comment.
this should be isnothing (empty container means empty, not "whatever")
| ) | ||
|
|
||
| # map old idxs of metabolites to new order of smaller system | ||
| midx_lookup = Dict( |
There was a problem hiding this comment.
kill indexes, we worked hard to finally eradicate the stupid indexing from everywhere.
this can be done by just using the original code and adding a conditional continue to the right spot
| let r = Symbol(rid), | ||
| rf = reference_flux[rid], | ||
| df = dGr0 + R * T * constraints.reactant_log_concentrations[r].value | ||
| dGr = (rf > 0 ? 1 : -1) * driving_forces[r].value |
There was a problem hiding this comment.
this is not numerically robust, ignores zero tolerance
| dGr0 = reaction_standard_gibbs_free_energies[rid], | ||
| dGr = dGr0 + R * T * constraints.log_concentration_stoichiometries[r].value | ||
|
|
||
| r => C.Constraint(dGr, C.Between(-Inf, Inf)) |
| DomainError( | ||
| constant_concentrations, | ||
| "unknown metabolites referenced by constant_concentrations", | ||
| "unknown metabolites referenced by constant_concentrations.", |
There was a problem hiding this comment.
why is this suddenly a sentence? (either have a normal error message or convert everything to sentences consistently)
| [`linear_parsimonious_flux_balance_analysis`](@ref) (computationally simplest | ||
| but the consistency is not guaranteed). | ||
|
|
||
| Internally, [`log_concentration_constraints`](@ref) is used to lay out the | ||
| base structure of the problem. | ||
| Internally, [`log_concentration_constraints`](@ref) is used to lay out the base |
There was a problem hiding this comment.
why is this reformatted? (check your textwidth settings, should be 80)
|
deleting this to make room for better things :) |
I changed some stuff here and there, but ended up with something that is very similar to what you had... Made the model simplification steps explicit though, and separated out the allocation steps.