refactor: transform production envelope to wide format with multiple carbon sources #587

Midnighter · 2017-08-29T21:15:26Z

So far this PR re-organizes the code for production_envelope a little bit and catches the potential ZeroDivisionError in mass_yield. However, I suggest the following further changes which is why I marked this PR as WIP.

Allow specification of more than one carbon source.
Transform the resulting data frame into wide format (the way it's done in cameo) meaning that maximum and minimum are separate columns.
Use the configured solver threshold to convert below threshold values to zero.

hredestig · 2017-08-30T12:55:15Z

On 2, being used to ggplot's interface, I find long format much more convenient as the column names remain fixed and then easier to write general plotting code. But, I guess that may be somewhat niche case and that wide format feels more familiar.

Midnighter · 2017-08-31T17:24:55Z

Actually, even for ggplot2 I find the wide variant more convenient because I opted to use geom_ribbon.

ggplot2::ggplot(total_df, ggplot2::aes(x = biomass,
                                       ymin = flux_minimum,
                                       ymax = flux_maximum,
                                       color = label,
                                       fill = label)) +
  ggplot2::geom_ribbon(alpha=0.3) +
  ggplot2::scale_color_brewer(name = "Model", palette = "Set1") +
  ggplot2::scale_fill_brewer(name = "Model", palette = "Set1") +
  ggplot2::labs(x = expression(paste("Growth [", g[DW], h^{-1}, "]")),
                y = expression(paste("Flux [", mmol, g[DW]^{-1}, h^{-1}, "]")))

Which can produce something like the following plot, for example.

hredestig · 2017-09-01T08:03:55Z

Sure that works as well, I find it easiest to know the right format when having tried to implement many different plots which I didn't do yet (e.g. to recycle code with plotting e.g. fva, however that should be done the best way.). Something to think of when implementing overall better plot functions for cobra. In the mean time, switch to wide format here totally fine with me.

Midnighter · 2017-09-06T09:23:39Z

Done
Done
TBD

Any other ideas that should be included here? I think testing could be expanded for sure.

codecov-io · 2017-09-06T11:51:43Z

Codecov Report

Merging #587 into devel will increase coverage by 0.01%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##            devel     #587      +/-   ##
==========================================
+ Coverage   71.43%   71.45%   +0.01%     
==========================================
  Files          65       65              
  Lines        8778     8784       +6     
  Branches     1483     1491       +8     
==========================================
+ Hits         6271     6277       +6     
  Misses       2239     2239              
  Partials      268      268

Impacted Files	Coverage Δ
cobra/test/test_flux_analysis.py	`86.33% <100%> (+0.08%)`	⬆️
cobra/core/model.py	`90.45% <66.66%> (+0.02%)`	⬆️
cobra/flux_analysis/phenotype_phase_plane.py	`68.12% <80.88%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea866f3...69efe6a. Read the comment docs.

hredestig · 2017-09-12T08:12:05Z

cobra/flux_analysis/phenotype_phase_plane.py

-        results['carbon_source'] = carbon_io[0].id
-    return results
+        with model:
+            model.solver.objective.direction = sense


This doesn't use context manager, perhaps better formulate as model.objective = .. ? Otherwise I guess will exit with objective changed to max if starting with min..

hredestig · 2017-09-12T08:13:45Z

cobra/flux_analysis/phenotype_phase_plane.py

+                    for rxn in reactions:
+                        point = grid.at[i, rxn.id]
+                        rxn.bounds = point, point
+                    model.slim_optimize()


perhaps obj_value = model.slim_optimize() and then use obj_value below

hredestig · 2017-09-12T08:15:27Z

cobra/flux_analysis/phenotype_phase_plane.py

+                    model.slim_optimize()
+                    if model.solver.status != OPTIMAL:
+                        continue
+                    if abs(model.solver.objective.value) < \


how about making the tolerance an optional argument and have thereby have this clipping documented?

hredestig · 2017-09-12T08:17:46Z

cobra/flux_analysis/phenotype_phase_plane.py

    try:
        return carbon_output_flux / carbon_input_flux
    except ZeroDivisionError:
        return nan


-def mass_yield(c_input_output):
-    """Gram product divided by gram of carbon input source
+def reaction_components(reaction, atom='C'):


element not atom.. And since you add this, perhaps do it all the way and allow user to calculate any element yield by having e.g. yield_element='N' as argument to production_envelope? Or do later and remove parametrisation here..

Removing parametrization here for now. It's a little annoying with column naming so unless there is a need for it I'd skip it.

Midnighter · 2017-09-12T10:03:10Z

This PR is now based on #588 and the new Model.objective_direction property.

hredestig · 2017-09-13T08:37:11Z

cobra/flux_analysis/phenotype_phase_plane.py

+                        grid.at[i, 'carbon_yield_{}'.format(direction)] = \
+                            total_yield([rxn.flux for rxn in c_input],
+                                        input_components,
+                                        model.solver.objective.value,


obj_val ?

Absolutely!

Midnighter added the WIP work in progress label Aug 29, 2017

Midnighter self-assigned this Aug 29, 2017

Midnighter changed the title ~~refactor: catch zero division in mass yield [WIP]~~ refactor: transform production envelope to wide format with multiple carbon sources Sep 6, 2017

Midnighter requested a review from hredestig September 6, 2017 17:03

hredestig suggested changes Sep 12, 2017

View reviewed changes

Midnighter force-pushed the fix/production_envelope branch from a0f25a6 to d4c81e7 Compare September 12, 2017 10:02

Midnighter force-pushed the fix/production_envelope branch 3 times, most recently from 01329b2 to f292c28 Compare September 12, 2017 14:34

Midnighter and others added 6 commits September 13, 2017 09:27

refactor: catch zero division in mass yield

1777002

refactor: use wide data frame

aec1f22

refactor: add checks using solver tolerance

e36d619

refactor: address Henning's comments

464d725

refactor: base on new property

e50d684

refactor: lowercase value only once

7777ea7

Midnighter force-pushed the fix/production_envelope branch from f292c28 to 7777ea7 Compare September 13, 2017 07:30

hredestig reviewed Sep 13, 2017

View reviewed changes

hredestig approved these changes Sep 13, 2017

View reviewed changes

fix: replace objective value with variable

69efe6a

Midnighter merged commit 996ba7b into devel Sep 13, 2017

Midnighter deleted the fix/production_envelope branch September 13, 2017 09:28

hredestig mentioned this pull request Sep 27, 2017

Allow multiple carbon sources in production envelope? #583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: transform production envelope to wide format with multiple carbon sources #587

refactor: transform production envelope to wide format with multiple carbon sources #587

Midnighter commented Aug 29, 2017 •

edited

Loading

hredestig commented Aug 30, 2017

Midnighter commented Aug 31, 2017 •

edited

Loading

hredestig commented Sep 1, 2017

Midnighter commented Sep 6, 2017 •

edited

Loading

codecov-io commented Sep 6, 2017 •

edited

Loading

hredestig Sep 12, 2017

hredestig Sep 12, 2017

Midnighter Sep 12, 2017

hredestig Sep 12, 2017

Midnighter Sep 12, 2017

hredestig Sep 12, 2017

Midnighter Sep 12, 2017

Midnighter commented Sep 12, 2017

hredestig Sep 13, 2017

Midnighter Sep 13, 2017

refactor: transform production envelope to wide format with multiple carbon sources #587

refactor: transform production envelope to wide format with multiple carbon sources #587

Conversation

Midnighter commented Aug 29, 2017 • edited Loading

hredestig commented Aug 30, 2017

Midnighter commented Aug 31, 2017 • edited Loading

hredestig commented Sep 1, 2017

Midnighter commented Sep 6, 2017 • edited Loading

codecov-io commented Sep 6, 2017 • edited Loading

Codecov Report

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

Midnighter commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Midnighter commented Aug 29, 2017 •

edited

Loading

Midnighter commented Aug 31, 2017 •

edited

Loading

Midnighter commented Sep 6, 2017 •

edited

Loading

codecov-io commented Sep 6, 2017 •

edited

Loading