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.

Sign up for GitHub

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

Jump to bottom

Fix factor 2 in sres2fim; Add fast biological test model (CR) #619

Merged
merged 23 commits into from
May 12, 2021
Merged

Fix factor 2 in sres2fim; Add fast biological test model (CR) #619

merged 23 commits into from
May 12, 2021

Conversation

yannikschaelte
Copy link
Member

@yannikschaelte yannikschaelte commented Apr 8, 2021 •
edited
Loading

Fixes #618
Fixes #660 to some degree (not replacement yet, but at least a content-wise fitting scenario ...)

@yannikschaelte yannikschaelte self-assigned this Apr 8, 2021
@codecov-io
Copy link

codecov-io commented Apr 8, 2021 •
edited
Loading

Codecov Report

Merging #619 (e9a3048) into develop (160c2a8) will decrease coverage by 53.66%.
The diff coverage is 24.91%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #619       +/-   ##
============================================
- Coverage    88.16%   34.50%   -53.67%     
============================================
  Files           79       93       +14     
  Lines         5257     5854      +597     
============================================
- Hits          4635     2020     -2615     
- Misses         622     3834     +3212
Impacted Files Coverage Δ
pypesto/ensemble/covariance_analysis.py 18.36% <0.00%> (-0.39%) ⬇️
pypesto/objective/aesara.py 0.00% <0.00%> (ø)
pypesto/objective/aggregated.py 25.49% <0.00%> (-74.51%) ⬇️
pypesto/objective/util.py 30.76% <0.00%> (-61.54%) ⬇️
pypesto/petab/__init__.py 0.00% <0.00%> (ø)
pypesto/petab/importer.py 0.00% <0.00%> (-85.90%) ⬇️
pypesto/petab/pysb_importer.py 0.00% <0.00%> (-100.00%) ⬇️
pypesto/visualize/parameters.py 14.14% <0.00%> (-77.78%) ⬇️
pypesto/visualize/waterfall.py 14.81% <0.00%> (-81.49%) ⬇️
pypesto/store/read_from_hdf5.py 51.32% <5.55%> (-6.57%) ⬇️
... and 101 more

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 392655f...e9a3048. Read the comment docs.

@FFroehlich FFroehlich self-requested a review April 13, 2021 01:23
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2021 •
edited
Loading

Codecov Report

Merging #619 (a1bd40e) into develop (9691588) will increase coverage by 0.24%.
The diff coverage is 96.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #619      +/-   ##
===========================================
+ Coverage    89.57%   89.82%   +0.24%     
===========================================
  Files           94       94              
  Lines         5997     6013      +16     
===========================================
+ Hits          5372     5401      +29     
+ Misses         625      612      -13
Impacted Files Coverage Δ
pypesto/objective/util.py 96.55% <96.00%> (+4.24%) ⬆️
pypesto/objective/history.py 90.45% <100.00%> (ø)
pypesto/objective/amici_calculator.py 93.33% <0.00%> (+1.66%) ⬆️
pypesto/sample/geweke_test.py 94.36% <0.00%> (+2.81%) ⬆️
pypesto/objective/amici_util.py 84.21% <0.00%> (+8.77%) ⬆️

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 9691588...a1bd40e. Read the comment docs.

@yannikschaelte yannikschaelte linked an issue May 4, 2021 that may be closed by this pull request
Missing factor of 2 in sres to fim #618
Closed
@jvanhoefer
Copy link
Member

What is the status here? :)

@yannikschaelte
Copy link
Member Author

realized that this would break quite a few things with amici, therefore need to refactor

@yannikschaelte
Copy link
Member Author

@jvanhoefer @FFroehlich it should work now. Changed history FVAL from chi2 to fval, apart from that everything should be consistent ...

@yannikschaelte yannikschaelte changed the title Fix factor 2 in sres2fim, fixes #618 Fix factor 2 in sres2fim; Add fast biological test model (CR) May 12, 2021
PaulJonasJost
PaulJonasJost approved these changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@PaulJonasJost PaulJonasJost 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 to me.

jvanhoefer
jvanhoefer approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@jvanhoefer jvanhoefer left a comment

Choose a reason for hiding this comment

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

Looks fine, have only minor remarks :)

pypesto/objective/util.py Show resolved Hide resolved
pypesto/objective/util.py Outdated Show resolved Hide resolved
pypesto/objective/util.py Outdated Show resolved Hide resolved
test/base/test_history.py
@@ -394,6 +430,42 @@ def test_trace_all_aggregated(self):
self.check_history()


class CRFunModeHistoryTest(HistoryTest):
Copy link
Member

Choose a reason for hiding this comment

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

To me it is not clear, where the difference to CRResModeHistoryTest is, isn't this the exactly same code? And also it is not clear to me, where it is decided whether to use ModeFun or ModeRes?

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 difference is the used optimizer -- once it will call residuals, once function values.

test/optimize/test_optimize.py Outdated Show resolved Hide resolved
test/util.py Outdated
Comment on lines 139 to 140
self.p_true: anp.ndarray = p_true
self.sigma: float = sigma
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
self.p_true: anp.ndarray = p_true
self.sigma: float = sigma
self.p_true: anp.ndarray = p_true
self.sigma: float = sigma

test/util.py Show resolved Hide resolved
test/util.py Outdated Show resolved Hide resolved
test/util.py Outdated Show resolved Hide resolved
test/util.py Outdated Show resolved Hide resolved
@yannikschaelte yannikschaelte merged commit c7c94e1 into develop May 12, 2021
@yannikschaelte yannikschaelte deleted the fixes_ys branch May 12, 2021 12:02
@yannikschaelte yannikschaelte mentioned this pull request May 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulJonasJost PaulJonasJost PaulJonasJost approved these changes

@jvanhoefer jvanhoefer jvanhoefer approved these changes

@FFroehlich FFroehlich Awaiting requested review from FFroehlich

Assignees

@yannikschaelte yannikschaelte

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing factor of 2 in sres to fim
7 participants
@yannikschaelte @codecov-io @codecov-commenter @jvanhoefer @FFroehlich @PaulJonasJost @dilpath