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

[Common] port_id and parallel_edges_ids are added to the graph for analysis #2206

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

daniil-lyakhov
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov commented Oct 18, 2023

Changes

port_id and parallel_edges_ids are added to the graph for analysis

image

Reason for changes

To test parallel_edges_ids and port_id in graph tests as well as the structural information.

Related tickets

Tests

@daniil-lyakhov daniil-lyakhov requested a review from a team as a code owner October 18, 2023 11:39
@github-actions github-actions bot added NNCF Common Pull request that updates NNCF Common NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF ONNX Pull requests that updates NNCF ONNX labels Oct 18, 2023
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #2206 (2ac6f5e) into develop (5e0837a) will decrease coverage by 0.23%.
Report is 7 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2206      +/-   ##
===========================================
- Coverage    36.63%   36.40%   -0.23%     
===========================================
  Files          486      486              
  Lines        43377    43460      +83     
===========================================
- Hits         15889    15821      -68     
- Misses       27488    27639     +151     
Files Coverage Δ
nncf/common/graph/graph.py 74.85% <100.00%> (+0.43%) ⬆️

... and 37 files with indirect coverage changes

@github-actions github-actions bot added the NNCF PT Pull requests that updates NNCF PyTorch label Oct 26, 2023
Copy link
Contributor

@vshampor vshampor left a comment

Choose a reason for hiding this comment

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

So now all of the reference structural graphs except for torch use the format with port IDs and shapes, except for torch. Either regenerate torch reference graphs to the new format, or keep the old format in the other frameworks and only extend the reference graphs with the "parallel_input_port_ids".

@@ -607,12 +607,12 @@ def get_graph_for_structure_analysis(self, extended: bool = False) -> nx.DiGraph
else:
attrs_edge["style"] = "solid"
label["shape"] = edge[NNCFGraph.ACTIVATION_SHAPE_EDGE_ATTR]
label[
"ports"
] = f"{edge[NNCFGraph.OUTPUT_PORT_ID_EDGE_ATTR]}\u2192{edge[NNCFGraph.INPUT_PORT_ID_EDGE_ATTR]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you tried to make the representation prettier, but even in 2023 there's no guarantee that every tool can handle Unicode well enough. Use -> to represent an arrow, that would be safer.

@openvino-nncf-ci openvino-nncf-ci added the API Public API-impacting changes label Nov 6, 2023
@daniil-lyakhov daniil-lyakhov force-pushed the dl/fix_get_graph_for_structural_analysis branch from 2fc08c9 to 11d3d66 Compare November 6, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Public API-impacting changes NNCF Common Pull request that updates NNCF Common NNCF ONNX Pull requests that updates NNCF ONNX NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PT Pull requests that updates NNCF PyTorch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants