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

Quick fix for summary methods with exchange objective #595

Merged
merged 2 commits into from Sep 16, 2017

Conversation

pstjohn
Copy link
Contributor

@pstjohn pstjohn commented Sep 13, 2017

Previously model.optimize could get passed a list of reactions in which one reaction was duplicated. This PR fixes #593 by making sure duplicate reactions are dropped prior to calling model.optimize.

@pstjohn pstjohn added the ready label Sep 13, 2017
@cdiener
Copy link
Member

cdiener commented Sep 13, 2017

Good catch! Do you have an idea why any of those can return duplicate reactions? Should not be possible based on my understanding...

@pstjohn
Copy link
Contributor Author

pstjohn commented Sep 13, 2017

Yeah, it only happens when the objective reactions also appear in the boundary reactions 😄

@cdiener
Copy link
Member

cdiener commented Sep 13, 2017

Ah, of course :) thanks

@Midnighter
Copy link
Member

Could you add a test case for this, too, please. I think it's a good idea to start collecting these edge cases so we're future proof.

@codecov-io
Copy link

codecov-io commented Sep 13, 2017

Codecov Report

Merging #595 into devel will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #595      +/-   ##
==========================================
+ Coverage   71.41%   71.46%   +0.05%     
==========================================
  Files          65       65              
  Lines        8784     8787       +3     
  Branches     1491     1491              
==========================================
+ Hits         6273     6280       +7     
+ Misses       2242     2239       -3     
+ Partials      269      268       -1
Impacted Files Coverage Δ
cobra/test/test_flux_analysis.py 86.41% <100%> (+0.08%) ⬆️
cobra/flux_analysis/summary.py 80.64% <100%> (ø) ⬆️
cobra/flux_analysis/sampling.py 91.59% <0%> (+1.68%) ⬆️

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 996ba7b...99aec19. Read the comment docs.

Copy link
Member

@cdiener cdiener left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@hredestig hredestig merged commit 4ece229 into opencobra:devel Sep 16, 2017
@hredestig hredestig removed the ready label Sep 16, 2017
@pstjohn pstjohn deleted the fix/summary_duplicate_index branch September 16, 2017 14:28
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.

model.summary() returns error after optimizing an exchange(or sink) reaction
5 participants