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

Allow for condition-specific overrides of dynamic parameters (Closes … #92

Merged
merged 6 commits into from
Apr 3, 2019

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Apr 2, 2019

#90)

  • Remove duplicate functions

  • Remove non-standard handling of constant parameters

  • Replace fixed parameter names by nominal values during mapping

  • Check override parameter scale where possible

  • Does not implement recursive overrides. Not sure if we would want to have that.

)

- Remove duplicate functions
- Remove non-standard handling of constant parameters
- Replace fixed parameter names by nominal values during mapping
- Check override parameter scale where possible
@codecov-io
Copy link

codecov-io commented Apr 2, 2019

Codecov Report

Merging #92 into develop will increase coverage by 2.17%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #92      +/-   ##
===========================================
+ Coverage    55.21%   57.39%   +2.17%     
===========================================
  Files            7        7              
  Lines          728      751      +23     
  Branches       153      165      +12     
===========================================
+ Hits           402      431      +29     
+ Misses         296      291       -5     
+ Partials        30       29       -1
Impacted Files Coverage Δ
petab/core.py 74.77% <80%> (+3.8%) ⬆️

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 eae08e2...3299856. Read the comment docs.

Copy link
Member

@yannikschaelte yannikschaelte 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, just some questions.

Could you test on e.g. the Boehm/Fujita model whether we still get the correct likelihood values?

petab/core.py Show resolved Hide resolved
petab/core.py Outdated Show resolved Hide resolved
for overridee_id in condition_df.columns:
if overridee_id == 'conditionName':
continue
if condition_df[overridee_id].dtype != 'O':
Copy link
Member

Choose a reason for hiding this comment

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

what is that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type of mixed-type or python objects in pandas

petab/core.py Show resolved Hide resolved
petab/core.py Outdated Show resolved Hide resolved

# in case both parameter are in parameter table, their scale
# must match.
if overridee_id in parameter_df.index \
Copy link
Member

Choose a reason for hiding this comment

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

Is the overridee_id not required to be in parameter_df? Where is that parameter-condition override documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the overridee_id not required to be in parameter_df?

Wasn't completely sure how to handle that. If a parameter is always overridden: does is still have to be in the parameter table? We don't require it for the output parameters, so I think we shouldn't require it here.

Where is that parameter-condition override documented?

The documentation has a "parameterId" in the sample table in "Condition table" section. Could be extended at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would then also not require the overridee id to be in the parameter table, since none of the columns makes sense really (boundaries, scale, ... all overridden)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would leave that in for now. This way things should be safe. To allow for non-matching parameter scale overrides, get_optimization_to_simulation_scale_mapping will also have to be adapted, I think.

petab/core.py Outdated Show resolved Hide resolved
petab/core.py Outdated Show resolved Hide resolved
yannikschaelte and others added 2 commits April 2, 2019 14:54
Co-Authored-By: dweindl <dweindl@users.noreply.github.com>
@dweindl
Copy link
Member Author

dweindl commented Apr 2, 2019

Could you test on e.g. the Boehm/Fujita model whether we still get the correct likelihood values?

Somebody's not trusting our unittests? :)

@yannikschaelte
Copy link
Member

Could you test on e.g. the Boehm/Fujita model whether we still get the correct likelihood values?

Somebody's not trusting our unittests? :)

never.

@yannikschaelte
Copy link
Member

Could you test on e.g. the Boehm/Fujita model whether we still get the correct likelihood values?

Somebody's not trusting our unittests? :)

never.

More seriously: In particular to check whether the get_dynamic_parameters thing works with the real-world models and pypesto/amici yet.

@dweindl
Copy link
Member Author

dweindl commented Apr 2, 2019

Boehm/Fujita work with parPE. PyPESTO will require some adaptations.

@dweindl dweindl merged commit fe3f146 into develop Apr 3, 2019
@dweindl dweindl deleted the feature_90_dynamic_parameter_overrides branch April 3, 2019 17:21
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

3 participants