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 SUNContext in ModelStateDerived copy ctor #2583

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Nov 10, 2024

When copying SUNDIALS objects, we need to make sure they have a valid SUNContext set. When copying ModelStateDerived, the SUNContext needs to be updated. Otherwise, dangling SUNContext pointers likely result in segfaults later on (#2579).

This was done only for a subset of objects. In particular the ones added only during #2505 were missing, because of other changes happening in parallel.

To make this less messy: Add set_ctx(.) functions all our sundials vector/matrix wrappers.

Closes #2579.

@dweindl dweindl changed the title Fix SUNContext in ModelStateDerived copyctr Fix SUNContext in ModelStateDerived copyctor Nov 10, 2024
When copying SUNDIALS objects, we need to make sure they have a valid `SUNContext` set.
When copying `ModelStateDerived`, the `SUNContext` needs to be updated. Otherwise,
dangling SUNContext pointers likely result in segfaults later on (AMICI-dev#2579).

This was done only for a subset of objects. In particular the ones added only during
AMICI-dev#2505 were missing, because of other changes
happening in parallel.

To make this less messy: Add `set_ctx(.)` functions all our sundials vector/matrix
wrappers.

Closes AMICI-dev#2579.
@dweindl dweindl marked this pull request as ready for review November 10, 2024 14:03
@dweindl dweindl requested a review from a team as a code owner November 10, 2024 14:03
@dweindl dweindl mentioned this pull request Nov 10, 2024
@dweindl dweindl self-assigned this Nov 10, 2024
@dweindl dweindl changed the title Fix SUNContext in ModelStateDerived copyctor Fix SUNContext in ModelStateDerived copy ctor Nov 10, 2024
@dweindl dweindl enabled auto-merge November 10, 2024 14:14
Copy link

codecov bot commented Nov 10, 2024

Codecov Report

Attention: Patch coverage is 84.37500% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.83%. Comparing base (2c362b4) to head (d7ee642).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
include/amici/model_state.h 88.46% 0 Missing and 3 partials ⚠️
include/amici/sundials_matrix_wrapper.h 66.66% 0 Missing and 1 partial ⚠️
include/amici/vector.h 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2583      +/-   ##
===========================================
+ Coverage    77.76%   77.83%   +0.07%     
===========================================
  Files          325      325              
  Lines        21862    21852      -10     
  Branches      1473     1456      -17     
===========================================
+ Hits         17000    17008       +8     
+ Misses        4835     4833       -2     
+ Partials        27       11      -16     
Flag Coverage Δ
cpp 73.74% <75.00%> (+0.10%) ⬆️
cpp_python 34.21% <0.00%> (+<0.01%) ⬆️
petab 37.16% <81.25%> (+0.07%) ⬆️
python 72.54% <75.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/amici/sundials_matrix_wrapper.h 76.13% <66.66%> (-0.34%) ⬇️
include/amici/vector.h 77.08% <66.66%> (-5.14%) ⬇️
include/amici/model_state.h 97.00% <88.46%> (+18.55%) ⬆️

@dweindl dweindl added this pull request to the merge queue Nov 10, 2024
Merged via the queue into AMICI-dev:develop with commit 5c70794 Nov 10, 2024
20 checks passed
@dweindl dweindl deleted the fix_2579 branch November 10, 2024 17:51
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.

2 participants