Skip to content

benchmarking script#607

Merged
jhdark merged 29 commits into
festim-dev:fenicsxfrom
jhdark:benchmarking
Oct 18, 2023
Merged

benchmarking script#607
jhdark merged 29 commits into
festim-dev:fenicsxfrom
jhdark:benchmarking

Conversation

@jhdark
Copy link
Copy Markdown
Collaborator

@jhdark jhdark commented Oct 16, 2023

Proposed changes

Performance benchmarking is a useful way to keep track of how updates to the code affect performance.

A benchmark script is proposed comparing the performance of a simulation of gas permeation in pure fenicsx and in festim. It can be run using;
python3 test/benchmark.py
If the performance of festim is more than 20% worse than the fenics script on average, the test will fail rasing an error:
ValueError: festim is {diff:.1f}% slower than fenics, current acceptble threshold of 20%

More sophisticated methods of continuous testing can be implemented in the future: https://github.com/ionelmc/pytest-benchmark.

Types of changes

What types of changes does your code introduce to FESTIM?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2ce0c38) 95.88% compared to head (b9aab59) 95.89%.
Report is 7 commits behind head on fenicsx.

Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #607      +/-   ##
===========================================
+ Coverage    95.88%   95.89%   +0.01%     
===========================================
  Files           10       10              
  Lines          267      268       +1     
===========================================
+ Hits           256      257       +1     
  Misses          11       11              
Files Coverage Δ
festim/hydrogen_transport_problem.py 97.52% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jhdark jhdark marked this pull request as ready for review October 17, 2023 16:37
@jhdark jhdark added the fenicsx Issue that is related to the fenicsx support label Oct 17, 2023
@RemDelaporteMathurin
Copy link
Copy Markdown
Collaborator

I think this would be more useful if you had the action fail if the benchmark was higher than a certain value

Copy link
Copy Markdown
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

  • move the benchmark script to test
  • change threshold

Comment thread benchmark.py Outdated
Comment thread benchmark.py Outdated
Comment thread benchmark.py
Copy link
Copy Markdown
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

One small refactoring change and then good to merge!

Comment thread test/benchmark.py Outdated
@jhdark jhdark merged commit 45c988f into festim-dev:fenicsx Oct 18, 2023
@jhdark jhdark deleted the benchmarking branch October 18, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fenicsx Issue that is related to the fenicsx support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants