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

fix: skip determination of boundary types when no boundary rxns exist #733

Merged
merged 6 commits into from
Jul 20, 2018

Conversation

ChristianLieven
Copy link
Contributor

Fixes #732

@cdiener
Copy link
Member

cdiener commented Jul 12, 2018

Nice catch. The original error you reported was in the heuristic to identify the external compartment, so that probably needs a fix as well...

@ChristianLieven
Copy link
Contributor Author

@cdiener Am I not sufficiently circumventing that heuristic by skipping find_external_compartment altogether with the check I've implemented in find_boundary_types? As far as I can tell that is the only place in the code where find_external_compartment is called. It uses boundary reactions to identify the external compartment, so not running it at all when there are no boundary reactions seemed like an elegant solution to me as opposed to handling the error there and then having to refactor all depending functions. I guess this is a conceptual question so I'm happy to follow whatever you feel is more sensible! :)

@codecov-io
Copy link

codecov-io commented Jul 13, 2018

Codecov Report

Merging #733 into devel will decrease coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #733      +/-   ##
==========================================
- Coverage   82.98%   82.98%   -0.01%     
==========================================
  Files          40       40              
  Lines        3867     3872       +5     
  Branches      882      883       +1     
==========================================
+ Hits         3209     3213       +4     
- Misses        458      459       +1     
  Partials      200      200
Impacted Files Coverage Δ
cobra/medium/boundary_types.py 88.88% <80%> (-1.44%) ⬇️

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 b1aca5b...49d705a. Read the comment docs.

@cdiener
Copy link
Member

cdiener commented Jul 13, 2018

I agree that it fixes the bug but find_boundary_compartment is still a public function in that API, so it should also throw an error if there are no boundary reactions since there might be someone using that function on its own.

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.

Nice, thanks!

@ChristianLieven
Copy link
Contributor Author

Oops saw that there was a mistyped bit of plaintext in my code. :S Removed that in the latest commit.

@Midnighter Midnighter merged commit 5cc0bbf into devel Jul 20, 2018
@Midnighter Midnighter deleted the fix/catch_index_errors branch July 20, 2018 09:54
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.

None yet

4 participants