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

Deterministic order of event assignments #2242

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Dec 15, 2023

Ensure event assignments targets are processed in deterministic order. Otherwise the ordering of state variables may change between subsequent model imports, which we'd like to avoid.

Closes #2241.

Ensure event assignments targets are processed in deterministic order.
Otherwise the ordering of state variables may change between subsequent
model imports, which we'd like to avoid.

Closes AMICI-dev#2241.
@dweindl dweindl requested a review from a team as a code owner December 15, 2023 18:27
@dweindl dweindl linked an issue Dec 15, 2023 that may be closed by this pull request
@dweindl dweindl self-assigned this Dec 15, 2023
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Merging #2242 (4b117d9) into develop (594b07e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2242      +/-   ##
===========================================
+ Coverage    76.64%   76.66%   +0.01%     
===========================================
  Files           91       91              
  Lines        15112    14998     -114     
===========================================
- Hits         11583    11498      -85     
+ Misses        3529     3500      -29     
Flag Coverage Δ
cpp 73.11% <ø> (ø)
cpp_python 37.08% <ø> (ø)
petab 53.74% <66.66%> (-0.04%) ⬇️
python 78.02% <100.00%> (+0.57%) ⬆️
sbmlsuite ∅ <ø> (∅)

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

Files Coverage Δ
python/sdist/amici/sbml_import.py 79.15% <100.00%> (+0.12%) ⬆️

... and 8 files with indirect coverage changes

@dweindl dweindl merged commit b61c030 into AMICI-dev:develop Dec 18, 2023
25 checks passed
@dweindl dweindl deleted the fix_2241 branch December 18, 2023 13:25
dweindl added a commit to dweindl/AMICI that referenced this pull request Dec 18, 2023
Ensure event assignments targets are processed in deterministic order.
Otherwise the ordering of state variables may change between subsequent
model imports, which we'd like to avoid.

Closes AMICI-dev#2241.
dweindl added a commit that referenced this pull request Dec 18, 2023
Improves model code generation by collapsing cases with identical statements.

I.e.
```
switch(a)
 case b:
 case c:
  statements;
  break;
```
instead of

```
switch(a)
 case b:
  statements;
  break;
 case c:
  statements;
  break;
```

For my current model of interest, containing many events, this significantly reduces the generated code:

E.g.:
```
 16K my_model/deltasx.cpp
6,6M my_model_old/deltasx.cpp
```

Overall, for this model, I got from 204201 LOC down to 7936 LOC (i.e. -96%).

* ..

* Apply suggestions from code review

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

* GHA: test python3.12 (#2179)

Run nightly tests also on python3.12

* Deterministic order of event assignments (#2242)

Ensure event assignments targets are processed in deterministic order.
Otherwise the ordering of state variables may change between subsequent
model imports, which we'd like to avoid.

Closes #2241.

* Fix AMICI hiding all warnings (#2243)

Previously, importing amici would result in all warnings of the program being hidden, due to `logging.captureWarnings(True)`:

```sh
$ python -c "import warnings; warnings.warn('bla');"
<string>:1: UserWarning: bla
$ python -c "import amici; import warnings; warnings.warn('bla');"
$
```

This can't be the desired default.

Changes:
* Default to not capturing warnings
* If warnings are to be captured, at least handle them by amici loggers

Closes ICB-DCM/pyPESTO#1252


---------

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
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.

Non-deterministic code generation
2 participants