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

[sampling] fix inhomogeneous sampling and avoid recursion error #556

Merged
merged 3 commits into from Jul 26, 2017

Conversation

Projects
None yet
4 participants
@cdiener
Member

cdiener commented Jul 24, 2017

Bug fixes

Gives a putative solution for recursion limit errors when using multiprocessing.Pool. Apparently arguments can not be a zip object. No idea why though...


Fixes bugs related to inhomogeneous sampling. Turns out the reprojection strategy was overly complicated and did not work well since it spans very little of the space. As a consequence inhomogeneous sampling often returned identical samples (fixes #558). Using delta = any_warmup - center as new direction already works for inhomogeneous problems since S*(any_warmup - center) = b - b = 0. This spans the search space much better and gives a much better distribution.

Minor fixes

The sampling problem now stores the nullspace rather than the entire projection (N * N.T) which reduces the memory footprint since it uses memory in the order of 2 * n_reaction * n_metabolites instead of 4 * n_reactions^2. Does not seem to influence sampling speed at all.

Log some minor issues the sampler resolves automatically as info for better debugging.

@cdiener cdiener added the ready label Jul 24, 2017

Show outdated Hide outdated config.sh Outdated
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 25, 2017

Codecov Report

Merging #556 into devel will increase coverage by 0.2%.
The diff coverage is 98%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel     #556     +/-   ##
=========================================
+ Coverage   71.11%   71.31%   +0.2%     
=========================================
  Files          65       65             
  Lines        8689     8715     +26     
  Branches     1477     1475      -2     
=========================================
+ Hits         6179     6215     +36     
+ Misses       2240     2234      -6     
+ Partials      270      266      -4
Impacted Files Coverage Δ
cobra/test/test_flux_analysis.py 85.77% <100%> (+0.87%) ⬆️
cobra/flux_analysis/sampling.py 91.26% <95.45%> (+4.3%) ⬆️

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 6ec1c14...9317a4d. Read the comment docs.

codecov-io commented Jul 25, 2017

Codecov Report

Merging #556 into devel will increase coverage by 0.2%.
The diff coverage is 98%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel     #556     +/-   ##
=========================================
+ Coverage   71.11%   71.31%   +0.2%     
=========================================
  Files          65       65             
  Lines        8689     8715     +26     
  Branches     1477     1475      -2     
=========================================
+ Hits         6179     6215     +36     
+ Misses       2240     2234      -6     
+ Partials      270      266      -4
Impacted Files Coverage Δ
cobra/test/test_flux_analysis.py 85.77% <100%> (+0.87%) ⬆️
cobra/flux_analysis/sampling.py 91.26% <95.45%> (+4.3%) ⬆️

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 6ec1c14...9317a4d. Read the comment docs.

@cdiener

This comment has been minimized.

Show comment
Hide comment
@cdiener

cdiener Jul 25, 2017

Member

Please don't merge yet, still debugging another issue...

Member

cdiener commented Jul 25, 2017

Please don't merge yet, still debugging another issue...

@cdiener cdiener changed the title from [sampling] Fix reset bug and reduce memory footprint to [WIP][sampling] Fix reset bug and reduce memory footprint Jul 25, 2017

@cdiener cdiener changed the title from [WIP][sampling] Fix reset bug and reduce memory footprint to [sampling] fix inhomogeneous sampling and avoid recursion error Jul 25, 2017

@cdiener

This comment has been minimized.

Show comment
Hide comment
@cdiener

cdiener Jul 25, 2017

Member

Okay, should be done now. Results that there was a much simpler solution for inhomogeneous sampling 😅

Member

cdiener commented Jul 25, 2017

Okay, should be done now. Results that there was a much simpler solution for inhomogeneous sampling 😅

@hredestig

LGTM, is it ready to merge? I can fix the lint if you like

@cdiener

This comment has been minimized.

Show comment
Hide comment
@cdiener

cdiener Jul 26, 2017

Member

Yes, ready to merge :)

Member

cdiener commented Jul 26, 2017

Yes, ready to merge :)

cdiener added some commits Jul 24, 2017

fix: reset bug and reduce memory footprint
* Bug fixes

Gives a putative solution for recursion limit errors when using
`multiprocessing.Pool`. Apparently arguments can not be a `zip`
object. No idea why though...

Fixes bugs related to inhomogeneous sampling. Turns out the reprojection
strategy was overly complicated and did not work well since it spans
very little of the space. As a consequence inhomogeneous sampling often
returned identical samples (fixes #558). Using `delta = any_warmup -
center` as new direction already works for inhomogeneous problems since
`S*(any_warmup - center) = b - b = 0`. This spans the search space much
better and gives a much better distribution.

* Minor fixes

The sampling problem now stores the nullspace rather than the entire
projection (N * N.T) which reduces the memory footprint since it uses
memory in the order of `2 * n_reaction * n_metabolites` instead of `4 *
n_reactions^2`. Does not seem to influence sampling speed at all.

Log some minor issues the sampler resolves automatically as info for
better debugging.

#556
fix: inhomogeneous sampling
* Bug fixes

Gives a putative solution for recursion limit errors when using
`multiprocessing.Pool`. Apparently arguments can not be a `zip`
object. No idea why though...

Fixes bugs related to inhomogeneous sampling. Turns out the reprojection
strategy was overly complicated and did not work well since it spans
very little of the space. As a consequence inhomogeneous sampling often
returned identical samples (fixes #558). Using `delta = any_warmup -
center` as new direction already works for inhomogeneous problems since
`S*(any_warmup - center) = b - b = 0`. This spans the search space much
better and gives a much better distribution.

* Minor fixes

The sampling problem now stores the nullspace rather than the entire
projection (N * N.T) which reduces the memory footprint since it uses
memory in the order of `2 * n_reaction * n_metabolites` instead of `4 *
n_reactions^2`. Does not seem to influence sampling speed at all.

Log some minor issues the sampler resolves automatically as info for
better debugging.

#556
refactor: dont project warmup points
and simplify code

@hredestig hredestig merged commit 4894128 into opencobra:devel Jul 26, 2017

4 checks passed

codecov/patch 98% of diff hit (target 71.11%)
Details
codecov/project 71.31% (+0.2%) compared to 6ec1c14
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

hredestig added a commit that referenced this pull request Jul 26, 2017

fix: reset bug and reduce memory footprint
* Bug fixes

Gives a putative solution for recursion limit errors when using
`multiprocessing.Pool`. Apparently arguments can not be a `zip`
object. No idea why though...

Fixes bugs related to inhomogeneous sampling. Turns out the reprojection
strategy was overly complicated and did not work well since it spans
very little of the space. As a consequence inhomogeneous sampling often
returned identical samples (fixes #558). Using `delta = any_warmup -
center` as new direction already works for inhomogeneous problems since
`S*(any_warmup - center) = b - b = 0`. This spans the search space much
better and gives a much better distribution.

* Minor fixes

The sampling problem now stores the nullspace rather than the entire
projection (N * N.T) which reduces the memory footprint since it uses
memory in the order of `2 * n_reaction * n_metabolites` instead of `4 *
n_reactions^2`. Does not seem to influence sampling speed at all.

Log some minor issues the sampler resolves automatically as info for
better debugging.

#556

hredestig added a commit that referenced this pull request Jul 26, 2017

fix: inhomogeneous sampling
* Bug fixes

Gives a putative solution for recursion limit errors when using
`multiprocessing.Pool`. Apparently arguments can not be a `zip`
object. No idea why though...

Fixes bugs related to inhomogeneous sampling. Turns out the reprojection
strategy was overly complicated and did not work well since it spans
very little of the space. As a consequence inhomogeneous sampling often
returned identical samples (fixes #558). Using `delta = any_warmup -
center` as new direction already works for inhomogeneous problems since
`S*(any_warmup - center) = b - b = 0`. This spans the search space much
better and gives a much better distribution.

* Minor fixes

The sampling problem now stores the nullspace rather than the entire
projection (N * N.T) which reduces the memory footprint since it uses
memory in the order of `2 * n_reaction * n_metabolites` instead of `4 *
n_reactions^2`. Does not seem to influence sampling speed at all.

Log some minor issues the sampler resolves automatically as info for
better debugging.

#556

@hredestig hredestig removed ready labels Jul 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment