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

Warn if ASA is used in combination with events #2097

Merged
merged 3 commits into from
May 26, 2023
Merged

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented May 17, 2023

#18

@dweindl dweindl requested a review from a team as a code owner May 17, 2023 16:33
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #2097 (3c94976) into develop (d301df6) will decrease coverage by 22.13%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #2097       +/-   ##
============================================
- Coverage    76.51%   54.38%   -22.13%     
============================================
  Files           81       33       -48     
  Lines        14853     5516     -9337     
============================================
- Hits         11365     3000     -8365     
+ Misses        3488     2516      -972     
Flag Coverage Δ
cpp ?
petab 54.38% <50.00%> (-0.01%) ⬇️
python ?
sbmlsuite ?

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

Impacted Files Coverage Δ
python/sdist/amici/swig_wrappers.py 66.26% <50.00%> (-29.94%) ⬇️

... and 64 files with indirect coverage changes

@dweindl dweindl enabled auto-merge May 17, 2023 19:01
@dweindl dweindl self-assigned this May 17, 2023
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.

‚ne‘ counts discontinuities in the rhs and x. Discontinuities in the rhs are fine with adjoint sensitivity analysis as long as the trigger function is not parameter dependent.

@dweindl
Copy link
Member Author

dweindl commented May 22, 2023

‚ne‘ counts discontinuities in the rhs and x. Discontinuities in the rhs are fine with adjoint sensitivity analysis as long as the trigger function is not parameter dependent.

Ok, thanks. As this is not apparent from the generated model module, and i don't feel like storing it there - how do we want to handle that?

  1. change the message and live with the warnings for non-parameter-dependent trigger functions
  2. rather warn once during model import
  3. leave it as it was before (no warning)

@FFroehlich
Copy link
Member

FFroehlich commented May 22, 2023

‚ne‘ counts discontinuities in the rhs and x. Discontinuities in the rhs are fine with adjoint sensitivity analysis as long as the trigger function is not parameter dependent.

Ok, thanks. As this is not apparent from the generated model module, and i don't feel like storing it there - how do we want to handle that?

  1. change the message and live with the warnings for non-parameter-dependent trigger functions
  2. rather warn once during model import
  3. leave it as it was before (no warning)

Hmm rephrase warning and state that it might not apply to the model at hand, i.e. 1.?

python/sdist/amici/swig_wrappers.py Outdated Show resolved Hide resolved
python/sdist/amici/swig_wrappers.py Outdated Show resolved Hide resolved
python/sdist/amici/swig_wrappers.py Outdated Show resolved Hide resolved
python/sdist/amici/swig_wrappers.py Outdated Show resolved Hide resolved
@FFroehlich FFroehlich mentioned this pull request May 26, 2023
Co-authored-by: Fabian Fröhlich <[email protected]>
@dweindl dweindl requested a review from FFroehlich May 26, 2023 09:23
@dweindl dweindl disabled auto-merge May 26, 2023 10:46
@dweindl dweindl merged commit 85c1f51 into develop May 26, 2023
@dweindl dweindl deleted the warn_asa_events branch May 26, 2023 10:47
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