-
Notifications
You must be signed in to change notification settings - Fork 41
Refactor of flow tools - XZ-corrections from pattern #407
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #407 +/- ##
==========================================
+ Coverage 85.60% 86.56% +0.95%
==========================================
Files 46 44 -2
Lines 6677 6126 -551
==========================================
- Hits 5716 5303 -413
+ Misses 961 823 -138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit updates the baseline while TeamGraphix#392 is not merged.
thierry-martinez
left a comment
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.
Looks good! Two minor comments.
tests/test_pattern.py
Outdated
| assert dict(xzc.x_corrections) == {} | ||
| assert dict(xzc.z_corrections) == {} |
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.
I think casting to dict is not necessary.
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.
Opss, you are right. I had used a defaultdict somewhere before but I changed everything to dict. Fixed in 06dad95
graphix/flow/core.py
Outdated
| og, x_corrections, z_corrections | ||
| ) # Raises an `XZCorrectionsError` if mappings are not well formed. | ||
|
|
||
| return XZCorrections(og, x_corrections, z_corrections, tuple(partial_order_layers)) |
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.
The casting to tuple is no longer necessary since _corrections_to_partial_order_layers returns a tuple.
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.
Fixed in 06dad95
thierry-martinez
left a comment
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.
LGTM! Thanks.
This commit introduces tooling to extract an
XZCorrectionsobject from aPattern. It drops all the remaining dependencies ongraphix.gflow. Additionally it fixes two unreported bugs (see discussion below).Description of changes
Introduced the following methods:
graphix.optimization.StandardizedPattern.extract_xzcorrectionsXZCorrections.from_measured_nodes_mapping.graphix.pattern.Pattern.extract_xzcorrectionsModified the constructor
XZCorrections.from_measured_nodes_mappingso that it doesn't need to create anx.DiGraphinstance. Instead it exploitsgraphix.flow._partial_order.compute_topological_generationsintroduced in Refactor of flow tools - Flow from pattern #393. This simplies the code in the public API and fixes an unreported bug in the method (see Code snippets section).Fixed an unreported bug in
OpenGraph.is_equal_structurallywhich failed to compare open graphs differing on the output nodes only.The modules
graphix.gflowandgraphix.find_pfloware removed. Note thatgraphix.visualizationdoes not fully exploit the new flow API but this is out of the scope of this PR.Method
graphix.Pattern.get_layersis removed (see bugPattern.get_layersassumes that inputs cannot be outputs #367) in favor of existinggraphix.Pattern.extract_partial_order_layers.Code snippets
The following correction maps and open graph are incorrectly parsed by the current implementation of
XZCorrections.from_measured_nodes_mappinginmaster. This is fixed in this PR.