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

Fix HierarchicalAmiciCalculator.__call__ not setting 'hess' in result #1161

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Oct 30, 2023

HierarchicalAmiciCalculator.__call__ needs to set 'hess' in the returned dict, also in case of simulation failures.

Closes #1160

…t upon simulation failure

`HierarchicalAmiciCalculator.__call__` needs to set `'hess'` in the result dict, also in case of simulation failures.

Closes #1160
@dweindl dweindl requested a review from dilpath as a code owner October 30, 2023 10:31
@dweindl dweindl self-assigned this Oct 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2023

Codecov Report

Merging #1161 (81613c1) into develop (160c2a8) will decrease coverage by 3.77%.
Report is 407 commits behind head on develop.
The diff coverage is 87.87%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #1161      +/-   ##
===========================================
- Coverage    88.16%   84.39%   -3.77%     
===========================================
  Files           79      148      +69     
  Lines         5257    11673    +6416     
===========================================
+ Hits          4635     9852    +5217     
- Misses         622     1821    +1199     
Files Coverage Δ
pypesto/C.py 100.00% <100.00%> (ø)
pypesto/__init__.py 100.00% <100.00%> (ø)
pypesto/engine/__init__.py 100.00% <100.00%> (ø)
pypesto/engine/multi_process.py 100.00% <100.00%> (+7.69%) ⬆️
pypesto/engine/multi_thread.py 100.00% <100.00%> (ø)
pypesto/engine/single_core.py 100.00% <100.00%> (ø)
pypesto/engine/task.py 100.00% <100.00%> (ø)
pypesto/ensemble/__init__.py 100.00% <100.00%> (ø)
pypesto/ensemble/task.py 100.00% <100.00%> (ø)
pypesto/hierarchical/__init__.py 100.00% <100.00%> (ø)
... and 138 more

Copy link
Contributor

@Doresic Doresic left a comment

Choose a reason for hiding this comment

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

Yes, I think for simulation failures it should return a nan hessian. I think I encountered a situation where that caused an issue if fides (with BFGS hessian, not analytical) is used with hierarchical optimization. So the changes as they are are good.

However, just to mention: I believe that the true hessian is not the same as the one given by AMICI, because of the hierarchical optimization of inner parameters. The gradient from AMICI is correct as the hierarchical parameters are optimal, so the derivatives cancel out. But I'm not sure that happens for the Hessian. It could even be correct, thinking about it now briefly, since the inner problem optimization is convex so for the optimum both first and second-order derivatives should be 0. But I'm not sure, would have to look at it in detail.

@dweindl
Copy link
Member Author

dweindl commented Oct 31, 2023

The gradient from AMICI is correct as the hierarchical parameters are optimal, so the derivatives cancel out. But I'm not sure that happens for the Hessian. It could even be correct, thinking about it now briefly, since the inner problem optimization is convex so for the optimum both first and second-order derivatives should be 0. But I'm not sure, would have to look at it in detail.

Would be great if you could double-check that. If not, we need to let Objective know that we can't provide the Hessian instead of happily providing a potentially incorrect one.

That's independent of this PR - will merge this already.

@dweindl dweindl merged commit 378f705 into develop Oct 31, 2023
18 checks passed
@dweindl dweindl deleted the fix_1160_hess branch October 31, 2023 10:03
This was referenced Nov 15, 2023
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.

HierarchicalAmiciCalculator.__call__ missing 'hess' in returned dict upon simulation failure
3 participants