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

KeyError and potential solution in sampling tutorial #28

Closed
adamkova opened this issue May 28, 2019 · 4 comments
Closed

KeyError and potential solution in sampling tutorial #28

adamkova opened this issue May 28, 2019 · 4 comments
Assignees
Labels

Comments

@adamkova
Copy link

Dear Pierre and colleagues,

First of all, thank you for the great tool, the neat support in the docs and the informative articles.

A minor issue/suggestion:
In tutorial_sampling.py, using model iJO1366.json, I got a KeyError on line 72, while applying flux directionality. Going into the code in /analysis/manipulation.py, I found that if I change the end of the code on line 70 from
backward_use.variable.lb = round(solution.x_dict[backward_use.name])
to
backward_use.variable.lb = round(solution.x_dict[backward_use.id])
and similarly for the other three lines as well, I can enforce the use of the raw id name, without the "BU_" and "FU_" prefixes.

Not sure if it was a common issue, but in case anyone else would run into the same, this solved my problem.

Keep up the great work!

Best, Adam

@psalvy
Copy link
Member

psalvy commented May 28, 2019

Good day Adam!

Thank you for the kind words :) We appreciate that people read our prose and use our tools !

Indeed, I forgot to adapt this part of the code to the new fields of the solution attribute. The problem of your solution is that backward_use.id will yield for example PGI instead of BU_PGI, and PGI is the variable name of the reaction PGI -- which might create some issues since that would mean bounding an integer with continuous values ;) (This also explains why you do not get a key error)

However, i replaced solution.x_dict by solution.raw, which yields the raw Series of variable values at solution - this should work. Try pulling from dev (commit b9f14d2) - and tell me if that solves your problem :)

Cheers,
Pierre

@adamkova
Copy link
Author

Hi Pierre,

Appreciate the quick response and the update! You are right, I was binding the integers inappropriately... After pulling the update, it works like a charm, thanks.

Subsequently, I found x_dicts to be replaced in /analysis/variability.py as well. Would the below changes make sense (as per your previous update)?
77: profiles[iter_count] = solution.x_dictprofiles[iter_count] = solution.raw
207: fluxes = solution.x_dict[reaction_id]fluxes = solution.raw[reaction_id]
210: deltag = pd.Series({x.id:solution.x_dict[x.name]deltag = pd.Series({x.id:solution.raw[x.name]

In the case of line 207, I am a bit confused about the deprecated x_dicts from cobrapy (which were suggested to be changed to fluxes) and these x_dicts that should be changed to raw. Can you maybe suggest an intuitive way to differentiate them? This pytfa - cobrapy integration and tracking the attributes/variables gets a bit tricky for me at certain points... :)

Thanks ahead!
Adam

psalvy added a commit that referenced this issue May 29, 2019
@psalvy
Copy link
Member

psalvy commented May 29, 2019

Good day Adam,

Thank you for pointing these out!
Yes, I kinda coded this in a hurry without proper documentation ... Here was my reasoning:

  • solution.fluxes in cobrapy is a transformed version of the solver output, as it actually calculates the net flux of each reaction by substracting the reverse variable value to the forward variable value. This should be used anytime one needs the actual flux value

  • solution.raw is an attribute I added to get a clear copy of the solver output. From there one can access the value at solution for all the variables of the problem. However, looking for a reaction ID in there will only give the forward flux. This should be used for any other variable than fluxes.

I did not want to remove fluxes from solution.raw because then, well, it would not be raw anymore ...
However if you have suggestions with respect to this I would gladly take them !
I should have corrected the last remnants of x_dict in the latest push (dev, commit a985ce8)

Let me know if that solved your plroblem, and I'll close the issue :)

Cheers,
Pierre

@adamkova
Copy link
Author

Good day to you too, Pierre,

Thank you for the new update and the detailed explanation. The pace at which you addressed these issues is impressive 👍.

This thumb rule about the use cases of fluxes and raw makes good sense to me now. I also agree that for completeness, solution.raw should contain everything.

The changes seem to have done the trick, the code regained its flow.

Regarding suggestions, I would not make any now. It is a big project, and I could see that it takes quite some time to get a good enough overview of what is happening under the hood. Momentarily, it seems more feasible to assume that you made all the right choices during coding. :)

With that, please feel free to close this issue.

Keep it up,
Adam

@psalvy psalvy closed this as completed May 29, 2019
psalvy added a commit that referenced this issue May 29, 2019
* MNT: Pypi details

* MNT: Adding veriable creation logs

* FIX: typo

* FIX: Issue #28, incorrect variable referencing

* TST: adding tests for some of the analysis functions

* FIX: more fixes from #28

* FIX: fixed transport reaction function

* TST: Better tests, with a smaller model for I/O

* TST/FIX: relative<- absolute paths for small_model resources

* FIX: typo in paths to small_model resources for tests

* VER: Bump v0.9.0-b2
@psalvy psalvy self-assigned this Jun 3, 2019
@psalvy psalvy added the bug label Jun 3, 2019
psalvy pushed a commit that referenced this issue Jun 7, 2019
* Dev: Incremental update (#29)

* MNT: Pypi details

* MNT: Adding veriable creation logs

* FIX: typo

* FIX: Issue #28, incorrect variable referencing

* TST: adding tests for some of the analysis functions

* FIX: more fixes from #28

* FIX: fixed transport reaction function

* TST: Better tests, with a smaller model for I/O

* TST/FIX: relative<- absolute paths for small_model resources

* FIX: typo in paths to small_model resources for tests

* VER: Bump v0.9.0-b2

* FIX: Update subclasses dynamically for the dict serialization.
psalvy added a commit that referenced this issue Jan 21, 2020
* Dev: Incremental update (#29)

* MNT: Pypi details

* MNT: Adding veriable creation logs

* FIX: typo

* FIX: Issue #28, incorrect variable referencing

* TST: adding tests for some of the analysis functions

* FIX: more fixes from #28

* FIX: fixed transport reaction function

* TST: Better tests, with a smaller model for I/O

* TST/FIX: relative<- absolute paths for small_model resources

* FIX: typo in paths to small_model resources for tests

* VER: Bump v0.9.0-b2

* VER: bump to v0.9.1

* Update LICENSE.txt

* Fixing a couple of minor bugs and added new variable class (#35)

Thank you @remidhum 

* FIX: fixed the apply_directionality function

solution.raw dataframe index does not contain the fwd and bwd use variables.

* ENH: added new binary variable class

This class is usefull to deal with inactive reactions in a model.

* ENH: deal with models without an objective function

Objective function is set to Zero (symbol("0") does not work!) if there is no defined objective function

* OOPS: change proper function ...

* FIX: fixed the failing test

As suggested, I added a check to determine what object type is passed.

* MNT: solution object type testing improved

* feat: TOO SLOW eQuilibrator integration

* refactor: make use of new changes on equilibrator_api (https://gitlab.com/elad.noor/equilibrator-api/merge_requests/13)

* chore: equilibrator_api as extra dependency

* fix: make thermo_data optional

* chore: test equilibrator_api integration

* chore: set equilibrator_api as extra dependency

* refactor: back to previous init to impose thermo_data argument

* refactor: build thermo data from equilibrator as external function

* fix: install equilibrator as package

Co-Authored-By: Moritz E. Beber <midnighter@posteo.net>

* FIX: correct typo on error attribute

* refactor: load equilibrator on import of file

* REFACTOR: move print to logger warning

* FIX: set lower bound version on equilibrator_cache compatibility

* FIX: remove f-string syntax

* FIX: test equilibrator compatibility only for >= py3.6

* FIX: try to overcome py3.5 problems

* REFACTOR: ignore equilibrator test collection on py<3.6

* FIX: collect tests properly

* DOCS: add script as tutorial for equilibrator integration

* refactor: move equilibrator test to another module

* REFACTOR: encapsulate equilibrator testing

* "REFACTOR: REVERT encapsulate equilibrator testing, it doesn't work"

This reverts commit edc240d.

* "REFACTOR: REVERT move equilibrator test to another module"

This reverts commit 2f256de.

* Update test_equilibrator.py

* Update conftest.py

* fix: require Py>=3.6 when for equilibrator extra

* fix: import on function

* fix: remove conftest, proper coverage

* FIX: move pytfa import to function

* DOCS: warn about python version for equilibrator integration

* REFACTOR: remove GLPK solver enforcement

* fix: avoid side effects on tmodel

* FIX: change equilibrator-cache version

* TEST: change extra dependency management before_install

* FIX: typo

* FIX: add equilibrator_api to build

* fix: change travis pytest command to properly account for coverage

* FIX: typo

* FIX: separate coverage from pytest

* FIX: call codecov

* FIX: give coverage a try

* FIX: correct coverage install and calls

* Revert "FIX: correct coverage install and calls"

This reverts commit c1dfe77.

* Revert "FIX: give coverage a try"

This reverts commit eda6365.

* Revert "FIX: call codecov"

This reverts commit 38399e3.

* Revert "FIX: separate coverage from pytest"

This reverts commit 43ba69b.

* Revert "FIX: typo"

This reverts commit f10f1f8.

* Revert "fix: change travis pytest command to properly account for coverage"

This reverts commit 2b7b257.

* FIX: collect equilibrator tests at the end

* FIX: try with edit mode

* TEST: change file name

Co-authored-by: realLCSB <31034133+realLCSB@users.noreply.github.com>
Co-authored-by: Pierre Salvy <psalvy@users.noreply.github.com>
Co-authored-by: RémiDhum <rdhumeaux@gmail.com>
Co-authored-by: Moritz E. Beber <midnighter@posteo.net>
psalvy added a commit that referenced this issue Mar 4, 2021
Bypassing Travis because failing on GLPK not sympy anymore. See PR #49 

* Dev: Incremental update (#29)

* MNT: Pypi details

* MNT: Adding veriable creation logs

* FIX: typo

* FIX: Issue #28, incorrect variable referencing

* TST: adding tests for some of the analysis functions

* FIX: more fixes from #28

* FIX: fixed transport reaction function

* TST: Better tests, with a smaller model for I/O

* TST/FIX: relative<- absolute paths for small_model resources

* FIX: typo in paths to small_model resources for tests

* VER: Bump v0.9.0-b2

* VER: bump to v0.9.1

* Update LICENSE.txt

* Fixing a couple of minor bugs and added new variable class (#35)

Thank you @remidhum 

* FIX: fixed the apply_directionality function

solution.raw dataframe index does not contain the fwd and bwd use variables.

* ENH: added new binary variable class

This class is usefull to deal with inactive reactions in a model.

* ENH: deal with models without an objective function

Objective function is set to Zero (symbol("0") does not work!) if there is no defined objective function

* OOPS: change proper function ...

* FIX: fixed the failing test

As suggested, I added a check to determine what object type is passed.

* MNT: solution object type testing improved

* LIC: Create CLAI

* Name correction (#41)

Thank you for this correction :) Since I copied this from Google scholar, maybe check that they got your name right!
Cheers,
Pierre

* Allow saving of imported mat files as json (#42)

* Allow saving of imported mat files as json

When importing .mat models, the membranePot type is numpy's int16. The change makes it a standard int so that it can be saved as json.

* convert to float

* VER: Bump to 0.9.2

* MNT: removing warnings from deprecated usage of logger.warn

* FIX: metabolite thermo data was not recovered upon serialization

* FIX: Equilibrator update + reqs for building Dockerfile

* FIX: newer pypi version

* FIX: actual newer pypi version

* Update README.rst

* Update utils.py

* Zero import

Co-authored-by: realLCSB <31034133+realLCSB@users.noreply.github.com>
Co-authored-by: Pierre Salvy <psalvy@users.noreply.github.com>
Co-authored-by: Pierre Salvy <pierre.salvy@epfl.ch>
Co-authored-by: RémiDhum <rdhumeaux@gmail.com>
Co-authored-by: Daniel F Hernandez <dan.fhg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants