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

Feature fixed par plist for AmiciObjective #59

Merged
merged 12 commits into from
Nov 13, 2018

Conversation

FFroehlich
Copy link
Contributor

@FFroehlich FFroehlich commented Nov 7, 2018

Modifies AmiciObjective to make use of Model.setParameterList() to reduce computational cost if a large number of parameters is fixed.

@ICB-DCM ICB-DCM deleted a comment Nov 7, 2018
@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #59 into master will increase coverage by 1.38%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #59      +/-   ##
=========================================
+ Coverage   85.12%   86.5%   +1.38%     
=========================================
  Files          23      23              
  Lines        1062    1097      +35     
=========================================
+ Hits          904     949      +45     
+ Misses        158     148      -10
Impacted Files Coverage Δ
pypesto/objective/history.py 98.23% <100%> (ø) ⬆️
pypesto/problem.py 72.83% <100%> (+4.93%) ⬆️
pypesto/objective/objective.py 87.31% <100%> (-0.75%) ⬇️
pypesto/objective/amici_objective.py 72.43% <92.15%> (+12.93%) ⬆️
pypesto/objective/options.py 88.88% <0%> (+7.4%) ⬆️

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 451a0c7...ba037f9. Read the comment docs.

@ICB-DCM ICB-DCM deleted a comment Nov 10, 2018
@FFroehlich
Copy link
Contributor Author

@yannikschaelte

@@ -86,6 +84,108 @@ def res(x, sensi_orders):
# extract parameter names from model
self.x_names = list(self.amici_model.getParameterNames())

def get_bound_fun(self):
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. I think that beforehand amici always had to compute sensis for all parameters, and using model.setParameterList() now, only those that are not fixed are computed?

Can you shortly document why get_bound_fun and rebind_fun are necessary? Just so we don't run into similar problems in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Will add documentation

Copy link
Member

Choose a reason for hiding this comment

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

@FFroehlich : In the doc you say for get_bound_fun(self) that

Generate a fun function that calls _call_amici with MODE_FUN. Defining
a non-class function that references self as a local variable will bind
the function to a copy of the current self object and will
accordingly not take future changes to self into account.

I currently need to work on related stuff here. I do not completely understand what the problem is. Could you shortly explain please? If I do the following:

class A:
    def __init__(self):
        self.x = 1
        def nested_fun(y):
            print(self.x, y)
        self.fun = nested_fun

a = A()
a.fun(2)  # gives [1 2]
a.x = 42
a.fun(2)  # gives [42 2]

I get the output I would expect, i.e. the references inside self are updated correctly.

@ICB-DCM ICB-DCM deleted a comment Nov 13, 2018
@FFroehlich FFroehlich merged commit 4f0dc2f into master Nov 13, 2018
@FFroehlich FFroehlich deleted the feature_fixed_par_plist branch November 13, 2018 14:57
m-philipps pushed a commit that referenced this pull request Jun 14, 2022
Allow creating Problem from pre-existing data frames* Allow creating Problem from pre-existing data frames

* Flake...

* Model name and sbml_file should not be mandatory

* Refactor Problem. Remove file names, pickle DataFrames and Model directly. See discussion #61

* Fix whitespace-checking issues, extend check (Closes #59)

* Make petablint more error-tolerant and prettify

* Speedup handle_missing_overrides for larger problems (229s->64s)

* Speedup handle_missing_overrides; match names a bit looser

* Doc. Fail.

* Use ctor in Problem.from_files
m-philipps pushed a commit that referenced this pull request Jun 14, 2022
…for_observable (fixes #59) (#60)

Revert "More informative error messages in case of wrongly set observable and noise parameters (Closes PEtab-dev/PEtab#118) (PEtab-dev/PEtab#155)"

This reverts commit 8fab85e.
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.

3 participants