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

Changes Required for DD Reordering to Minimize Runtime #407

Draft
wants to merge 80 commits into
base: main
Choose a base branch
from

Conversation

Claudiaaaaaaaaa
Copy link

@Claudiaaaaaaaaa Claudiaaaaaaaaa commented Jul 24, 2024

Description

The running time of DDs depends on the order of the nodes in the diagram. In the package DDsim we implement a tool that tries to find the optimal or near-optimal order of nodes resulting in the minimal running time. All changes required in MQT-core are contained in this PR.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

src/DDMinimizer.cpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
Claudiaaaaaaaaa and others added 3 commits July 25, 2024 13:27
Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: ClaudiaDuenzinger <50296598+Claudiaaaaaaaaa@users.noreply.github.com>
Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: ClaudiaDuenzinger <50296598+Claudiaaaaaaaaa@users.noreply.github.com>
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 94.49541% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.5%. Comparing base (392844e) to head (44f2ec0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/DDMinimizer.cpp 94.4% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #407     +/-   ##
=======================================
+ Coverage   92.3%   92.5%   +0.1%     
=======================================
  Files         37      38      +1     
  Lines       2735    2950    +215     
  Branches     310     365     +55     
=======================================
+ Hits        2527    2730    +203     
- Misses       208     220     +12     
Flag Coverage Δ
cpp 94.2% <94.4%> (+<0.1%) ⬆️
python 87.4% <ø> (ø)
Files with missing lines Coverage Δ
src/DDMinimizer.cpp 94.4% <94.4%> (ø)

... and 6 files with indirect coverage changes

Claudiaaaaaaaaa and others added 4 commits August 13, 2024 13:46
Signed-off-by: ClaudiaDuenzinger <50296598+Claudiaaaaaaaaa@users.noreply.github.com>
Signed-off-by: ClaudiaDuenzinger <50296598+Claudiaaaaaaaaa@users.noreply.github.com>
Copy link
Collaborator

@ystade ystade left a comment

Choose a reason for hiding this comment

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

Hi @Claudiaaaaaaaaa, many thanks for this PR. Overall it looks very good already, especially, all the test pass already. That is really good. I went through your code in quite some detail and left a couple of comments. At first glance it looks like a lot but some you can just apply by clicking on accept if you agree. Once again, thank you for your effort. Your work is highly appreciated.

include/DDMinimizer.hpp Outdated Show resolved Hide resolved
include/DDMinimizer.hpp Outdated Show resolved Hide resolved
include/DDMinimizer.hpp Outdated Show resolved Hide resolved
include/DDMinimizer.hpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
src/DDMinimizer.cpp Outdated Show resolved Hide resolved
@burgholzer
Copy link
Member

burgholzer commented Oct 14, 2024

@Claudiaaaaaaaaa
Don't worry too much about the failing coverage uploads here at the moment. These are due to your PR originating from a fork, which, apparently, doesn't give the GitHub token the necessary permissions for uploads based on OIDC.
I'll look into whether this can be fixed.

Edit: This should hopefully be fixed by #448.
Guess we will see once that PR is in and you can merge the main branch into the PR here.

Edit 2: apparently, it wasn't fixed 😢

(mainly to trigger CI with the new workflows)

Signed-off-by: burgholzer <burgholzer@me.com>
@burgholzer
Copy link
Member

Ok, the CI errors should be resolved now. They were due to a regression in the codecov action that would request an OIDC token even from forks (which do not have the right permissions).
To trigger CI, I took the liberty to slightly simplify the test circuits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants