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

new 'gatecost' heuristic implemented #105

Merged
merged 30 commits into from
Feb 1, 2023
Merged

new 'gatecost' heuristic implemented #105

merged 30 commits into from
Feb 1, 2023

Conversation

AlexPloier
Copy link
Contributor

@AlexPloier AlexPloier commented Jul 25, 2022

for tcad journal final version, so the code is up-to-date w. the paper

@AlexPloier AlexPloier changed the title commits that include the new benchmark tests of the tcad version---si… new 'gatecost' heuristic implemented Jul 25, 2022
@burgholzer
Copy link
Member

Please resolve the conflicts reported by git and check that CI passes on your changes. Otherwise, I can't give this a proper review.

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.6%. Comparing base (357fb74) to head (7f83797).
Report is 345 commits behind head on main.

Files with missing lines Patch % Lines
src/PathSimulator.cpp 91.8% 3 Missing ⚠️
include/PathSimulator.hpp 94.4% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #105     +/-   ##
=======================================
+ Coverage   87.4%   87.6%   +0.1%     
=======================================
  Files         20      20             
  Lines       2036    2083     +47     
  Branches     358     367      +9     
=======================================
+ Hits        1781    1826     +45     
- Misses       255     257      +2     

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

@burgholzer
Copy link
Member

Please consider extending the C++ tests to cover your changes and assert that everything works properly.

Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Many thanks for the pull request.
I just quickly managed to go through all the changes and I believe there is still some work left to be done before this can be merged.
You can find detailed comments below. Perhaps the main points can be summarized as follows:

  • consider improving the handling of the gate_cost heuristic as I believe the current way it is implemented limits its performance to some degree (probably the most critical out of the requests for changes)
  • please remove any leftovers from development or debugging that are no longer needed
  • please do not write test for the sake of improving the number reported by codecov, but for actually testing the functionality of your changes/new features
  • please avoid to duplicate code and refactor appropriately to extract common functionality

include/PathSimulator.hpp Outdated Show resolved Hide resolved
include/PathSimulator.hpp Outdated Show resolved Hide resolved
include/PathSimulator.hpp Outdated Show resolved Hide resolved
include/PathSimulator.hpp Outdated Show resolved Hide resolved
include/PathSimulator.hpp Outdated Show resolved Hide resolved
src/PathSimulator.cpp Outdated Show resolved Hide resolved
src/PathSimulator.cpp Outdated Show resolved Hide resolved
src/PathSimulator.cpp Outdated Show resolved Hide resolved
src/PathSimulator.cpp Outdated Show resolved Hide resolved
test/python/taskbasedsimulator/benchmark.py Outdated Show resolved Hide resolved
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

These changes look mostly good to me and should be safe to merge. We can revisit later anyway if something pops up. Just one point where I am not quite sure where that change is coming from. Maybe you have the answer to that.

test/python/taskbasedsimulator/benchmark.py Show resolved Hide resolved
@hillmich hillmich merged commit 4e834fa into main Feb 1, 2023
@hillmich hillmich deleted the task_based_simulator branch February 1, 2023 07:57
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.

3 participants