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 custom llh #656

Merged
merged 21 commits into from
Mar 20, 2019
Merged

Feature custom llh #656

merged 21 commits into from
Mar 20, 2019

Conversation

yannikschaelte
Copy link
Member

Allow to specify different cost functions (not only a normal distribution, the former only thing), normal staying the default.

So far, 6 fixed distributions (normal, laplace, on lin/log/log10 scale) are supported.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #656 into develop will increase coverage by 0.04%.
The diff coverage is 90.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #656      +/-   ##
===========================================
+ Coverage     72.9%   72.94%   +0.04%     
===========================================
  Files           48       48              
  Lines         6946     6972      +26     
===========================================
+ Hits          5064     5086      +22     
- Misses        1882     1886       +4
Flag Coverage Δ
#cpp 69.49% <ø> (ø) ⬆️
#python 83.49% <90.62%> (+0.01%) ⬆️
Impacted Files Coverage Δ
python/amici/sbml_import.py 82.27% <90.62%> (+0.43%) ⬆️
python/amici/__init__.py 83.63% <0%> (-1.82%) ⬇️

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 5bd7712...034390b. Read the comment docs.

@AMICI-dev AMICI-dev deleted a comment Mar 19, 2019
noise_distributions = {}
else:
# Ensure no non-existing observableIds have been specified
# (no problem here, bu usually an upstream bug)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# (no problem here, bu usually an upstream bug)
# (no problem here, but usually an upstream bug)

@dweindl dweindl requested a review from FFroehlich March 19, 2019 15:44
Copy link
Member

@FFroehlich FFroehlich left a comment

Choose a reason for hiding this comment

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

Would be awesome to have tests, not sure wheter amici will be able to compute dJydy for all of those functions,.

@AMICI-dev AMICI-dev deleted a comment Mar 19, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 19, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 20, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 20, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 20, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 20, 2019
@yannikschaelte
Copy link
Member Author

@FFroehlich Can you have another look? I added a basic test now (can be improved if that is better). Indeed, testing proves useful, because the abs terms beforehand did not work. Now it works (tested also on a few own models).
I had to tell sympy that the needed variables are all real-valued to get the correct derivative of abs -> sign.
But I think that makes sense anyway?

@FFroehlich
Copy link
Member

Awesome, looks good!

@FFroehlich FFroehlich mentioned this pull request Mar 20, 2019
@AMICI-dev AMICI-dev deleted a comment Mar 20, 2019
@yannikschaelte yannikschaelte merged commit 5541ea7 into develop Mar 20, 2019
@yannikschaelte yannikschaelte deleted the feature_custom_llh branch March 20, 2019 21: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.

3 participants