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

update LQ generator vector insert order #1009

Merged
merged 6 commits into from
Nov 27, 2024
Merged

Conversation

josephleekl
Copy link
Contributor

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
Currently in GateImplementationsLM.hpp some generator functions performs two std::vector insertions to the same vector, both to the beginning of the destination vectors.

Description of the Change:
The order is swapped, and the second vector is inserted to the end of the vector. This should be more performant.

Benefits:

Possible Drawbacks:

Related GitHub Issues:

[sc-78933]

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.58%. Comparing base (db137bf) to head (c2f7dcd).
Report is 47 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1009   +/-   ##
=======================================
  Coverage   91.58%   91.58%           
=======================================
  Files         176      176           
  Lines       24535    24535           
=======================================
  Hits        22471    22471           
  Misses       2064     2064           

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

@josephleekl josephleekl marked this pull request as ready for review November 26, 2024 22:26
@josephleekl josephleekl requested review from maliasadi, multiphaseCFD, LuisAlfredoNu and AmintorDusko and removed request for maliasadi November 26, 2024 22:26
@josephleekl josephleekl added ci:build_wheels Activate wheel building. urgent Mark a pull request as high priority labels Nov 26, 2024
@josephleekl josephleekl changed the title update vector insert order update LQ generator vector insert order Nov 26, 2024
Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Good job finding and fixing this performance regression.

Copy link
Contributor

@LuisAlfredoNu LuisAlfredoNu left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @josephleekl

@multiphaseCFD
Copy link
Member

Thanks @josephleekl for getting this work done in this PR.

@josephleekl josephleekl merged commit 1177ae9 into master Nov 27, 2024
91 of 92 checks passed
@josephleekl josephleekl deleted the lq_wire_insert_order branch November 27, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:build_wheels Activate wheel building. urgent Mark a pull request as high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants