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

bug fixes #273

Merged
merged 7 commits into from
Sep 8, 2022
Merged

bug fixes #273

merged 7 commits into from
Sep 8, 2022

Conversation

jd-lara
Copy link
Member

@jd-lara jd-lara commented Sep 7, 2022

closes #271
closes #146

It also closes an issue with simulation reset that wasn't working correctly

@jd-lara jd-lara added the bug Something isn't working label Sep 7, 2022
@jd-lara jd-lara requested a review from rodrigomha September 7, 2022 23:58
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

Performance Results

Version Precompile Time
Master 10.666624873
This Branch 10.279763492
Version Execute Time
Master-Build ResidualModel 14.122944492
Master-Execute ResidualModel 77.80324982
Master-Build MassMatrixModel 1.248934112
Master-Execute MassMatrixModel 103.762706818
This Branch-Build ResidualModel 14.098837712
This Branch-Execute ResidualModel 76.909383651
This Branch-Build MassMatrixModel 1.324593395
This Branch-Execute MassMatrixModel 102.952944907

ResidualModel and MassMatrixModel performance results should be compared between versions and not between models due to the execution order of the tests

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #273 (b756ad3) into master (3eddbc3) will increase coverage by 0.63%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   89.96%   90.60%   +0.63%     
==========================================
  Files          65       64       -1     
  Lines        7113     7064      -49     
==========================================
+ Hits         6399     6400       +1     
+ Misses        714      664      -50     
Flag Coverage Δ
unittests 90.60% <68.42%> (+0.63%) ⬆️

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

Impacted Files Coverage Δ
src/base/definitions.jl 100.00% <ø> (ø)
src/base/simulation.jl 87.92% <62.50%> (-0.81%) ⬇️
src/base/jacobian.jl 95.77% <100.00%> (ø)
src/base/simulation_inputs.jl 93.10% <100.00%> (ø)
src/base/simulation_model.jl
src/base/frequency_reference.jl 88.23% <0.00%> (+0.73%) ⬆️
src/base/perturbations.jl 58.57% <0.00%> (+9.17%) ⬆️

@jd-lara
Copy link
Member Author

jd-lara commented Sep 8, 2022

The additional coverage is done in #274

Copy link
Contributor

@rodrigomha rodrigomha 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. We may want in the future add a test that reset the simulation by running the same execute two times.

@rodrigomha rodrigomha merged commit 1853567 into master Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address warning from keywords passed to solve Re-run simulation without reset
2 participants