-
Notifications
You must be signed in to change notification settings - Fork 24
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
Introduce input sort #183
base: main
Are you sure you want to change the base?
Introduce input sort #183
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
- Coverage 94.55% 94.25% -0.30%
==========================================
Files 83 87 +4
Lines 7581 8183 +602
==========================================
+ Hits 7168 7713 +545
- Misses 413 470 +57
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your effort with this contribution! Thus far, I have looked at the inverter balancing part in detail and had a quick look at the tests. Since my comments also point you toward more general topics that apply to all files, I would like to give you the opportunity to address them in all of your files, before I have a look at the remaining source code.
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
GitHub told me that i have to fetch some differences and now there is also a commit in this branch called: [Merge branch 'Introduce_Input_Sort' of], where some changes from your repository (i think) are included. Maybe have a quick look at it if everything is fine there. |
That seems fine 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your adjustments. Looks a lot better already!
As before, let's iterate a few files in detail while you can apply the lessons learned to the others as well.
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/network_transformation/inverter_balancing.hpp
Outdated
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
- Coverage 96.04% 95.87% -0.17%
==========================================
Files 102 106 +4
Lines 10101 10720 +619
==========================================
+ Hits 9701 10278 +577
- Misses 400 442 +42
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
include/fiction/algorithms/physical_design/ortho_ordering_network.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
Do you want to have another look. Now all CIs pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your hard work on this PR 🙏 We're getting closer. There are still some issues to resolve here and there, but they are mainly concerning documentation.
include/fiction/algorithms/physical_design/ortho_ordering_network.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/physical_design/ortho_ordering_network.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/physical_design/ortho_ordering_network.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/physical_design/ortho_ordering_network.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion: Instead of doing a whole new command, you could expose input ordering as a flag for ortho
if you wanted. It would reduce code duplication. If you rather have it this way, I'm okay with that, too.
CHECK(nodes_order == expected_nodes_order); | ||
} | ||
|
||
TEST_CASE("Input_ordering consider all pis aig network", "[input-ordering-view]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other network types? You could make this a TEMPLATE_TEST_CASE
to test for multiple implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the network type changes also the nodes change, so also the ranking of pis can change.
Description
Network_ordering utilizes following files:
Checklist: