Save port positions in the elk graph - #20
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7f0ce2c to
0e8326c
Compare
0e8326c to
9752500
Compare
| task_io_positions: TaskIOPositions = svg_task_group.extract_io_positions() | ||
| elk_graph: ElkGraph = convert_ewoks_to_elk_graph( | ||
| ewoks_graph, task_sizes, task_io_positions | ||
| ) | ||
|
|
||
| pprint(dict(ewoks_graph.graph.nodes(data=True))) | ||
| pprint(list(ewoks_graph.graph.edges(data=True))) | ||
| pprint(task_sizes) | ||
| pprint(task_io_positions) | ||
| pprint(elk_graph) |
There was a problem hiding this comment.
New method to extract io_positions returning a TaskIOPositions.
Example:
{
'task1': [
TaskIOPosition(name='a', io_type='input', x=0.0, y=20.0),
TaskIOPosition(name='b', io_type='input', x=0.0, y=28.0),
TaskIOPosition(name='delay', io_type='input', x=0.0, y=36.0),
TaskIOPosition(name='result', io_type='output', x=39.56, y=47.0)],
'task2': [
TaskIOPosition(name='a', io_type='input', x=0.0, y=20.0),
TaskIOPosition(name='b', io_type='input', x=0.0, y=28.0),
TaskIOPosition(name='delay', io_type='input', x=0.0, y=36.0),
TaskIOPosition(name='result', io_type='output', x=39.56, y=47.0)]
}
The elk_graph representation now includes the TaskIOPositions (called "Ports" in ELK semantic).
Example:
{'children': [{'height': 55.0,
'id': 'task1',
'layoutOptions': {'org.eclipse.elk.portConstraints': 'FIXED_POS'},
'ports': [{'height': 0,
'id': 'task1.input.a',
'layoutOptions': {'org.eclipse.elk.port.borderOffset': 0,
'org.eclipse.elk.port.index': 0,
'org.eclipse.elk.port.side': 'WEST'},
'width': 0,
'x': 0.0,
'y': 20.0},
....
{'height': 0,
'id': 'task1.output.result',
'layoutOptions': {'org.eclipse.elk.port.borderOffset': 0,
'org.eclipse.elk.port.index': 0,
'org.eclipse.elk.port.side': 'EAST'},
'width': 0,
'x': 39.56,
'y': 47.0}],
....
'edges': [{'id': 'edge_0_task1_task3',
'sources': ['task1.output.result'],
'targets': ['task3.input.a']},
{'id': 'edge_1_task2_task4',
'sources': ['task2.output.result'],
'targets': ['task4.input.a']},
{'id': 'edge_2_task3_task5',
'sources': ['task3.output.result'],
'targets': ['task5.input.a']},
{'id': 'edge_3_task4_task5',
'sources': ['task4.output.result'],
'targets': ['task5.input.b']},
{'id': 'edge_4_task5_task6',
'sources': ['task5.output.result'],
'targets': ['task6.input.a']}],
| for mapping in link_attrs.get("data_mapping", []): | ||
| source_output = mapping.get("source_output") | ||
| if not source_output: | ||
| warnings.warn( |
There was a problem hiding this comment.
I'm also ignoring links that have source_input but no source_output in the mapping.
There was a problem hiding this comment.
What is source_input? Did you mean target_input?
How is it possible to have a link that does not have a target and a source?
There was a problem hiding this comment.
Sorry, I meant the mapping has a target_input but no source_ouput
From the doc : ewokscore :
data_mapping (optional): Describe data transfer from source outputs to target input arguments. For example:
{
"data_mapping": [{"source_output": "result",
"target_input": "a"}]
}If "source_output" is None or missing, the complete output of the source will be passed to the corresponding "target_input" or the target.
There was a problem hiding this comment.
Gotcha. Didn't even know this was a thing
loichuder
left a comment
loichuder
left a comment
There was a problem hiding this comment.
I feel like some code would read better if we deal with inputs and outputs separately (rather than keeping a io_type flag). I identified already one place where it would simplify the code.
What do think?
| for mapping in link_attrs.get("data_mapping", []): | ||
| source_output = mapping.get("source_output") | ||
| if not source_output: | ||
| warnings.warn( |
There was a problem hiding this comment.
What is source_input? Did you mean target_input?
How is it possible to have a link that does not have a target and a source?
| def _convert_io_positions_to_elk_ports( | ||
| task_id: str, io_positions: list[TaskIOPosition] | ||
| task_id: str, io_positions: IOPositions | ||
| ) -> list[ElkPort]: | ||
| ports = _convert_positions_to_elk_ports( | ||
| io_positions.inputs, | ||
| id_prefix=f"{task_id}.input", | ||
| elk_port_side="WEST", | ||
| ) | ||
| ports.extend( | ||
| _convert_positions_to_elk_ports( | ||
| io_positions.outputs, | ||
| id_prefix=f"{task_id}.output", | ||
| elk_port_side="EAST", | ||
| ) | ||
| ) | ||
| return ports | ||
|
|
||
|
|
There was a problem hiding this comment.
A new split between input/output allows us to separate the two calls and avoid the constants
| y: float | ||
|
|
||
|
|
||
| class IOPositions(NamedTuple): |
There was a problem hiding this comment.
Instead of having a flag for input/output, we just contain both directly in a single structure
There was a problem hiding this comment.
Personally, I would not used a tuple but instead passed inputs and outputs separately down the line.
But I understand this is up to personal preference.
| def get_io_positions(self) -> IOPositions: | ||
| """Return the task-relative positions of the task inputs and outputs.""" | ||
| return IOPositions( | ||
| inputs=self._get_group_io_positions(self._inputs), | ||
| outputs=self._get_group_io_positions(self._outputs), | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _get_group_io_positions(group: SvgTaskIOGroup) -> list[TaskIOPosition]: | ||
| positions: list[TaskIOPosition] = [] | ||
| for io in group.elements: | ||
| x = group.translation.x + io.translation.x | ||
| y = group.translation.y + io.translation.y | ||
| positions.append(TaskIOPosition(name=io.name, x=x, y=y)) | ||
| return positions |
There was a problem hiding this comment.
New version of the get_io_position with the I/O splits.
Two simple method calls.
Co-authored-by: Loïc Huder <42204205+loichuder@users.noreply.github.com>
| y: float | ||
| width: float | ||
| height: float | ||
| layoutOptions: dict[str, Any] |
There was a problem hiding this comment.
Should be in snake case I think:
| layoutOptions: dict[str, Any] | |
| layout_options: dict[str, Any] |
There was a problem hiding this comment.
Actually, there is already layoutOptions (camel case) in ElkGraph so let's leave it like that for now.
| "width": width, | ||
| "height": height, |
There was a problem hiding this comment.
The tuple unpacking can be prone to mistake (it is easy to misattribute width and height).
By inlining the attribute access, we keep things explicit and less error-prone:
| "width": width, | |
| "height": height, | |
| "width": task_sizes[task_id].width, | |
| "height": task_sizes[task_id].height, |
|
|
||
| for mapping in link_attrs.get("data_mapping", []): | ||
| source_output = mapping.get("source_output") | ||
| if not source_output: |
There was a problem hiding this comment.
| if not source_output: | |
| if source_output is None: |
| for mapping in link_attrs.get("data_mapping", []): | ||
| source_output = mapping.get("source_output") | ||
| if not source_output: | ||
| warnings.warn( |
There was a problem hiding this comment.
Gotcha. Didn't even know this was a thing
| y: float | ||
|
|
||
|
|
||
| class IOPositions(NamedTuple): |
There was a problem hiding this comment.
Personally, I would not used a tuple but instead passed inputs and outputs separately down the line.
But I understand this is up to personal preference.
2 participants
Tip
The Feature
Note
Scope & Boundaries
TaskIOPositionvalues and converting them into fixedWESTinput andEASToutput ports withx/ycoordinatesmap_all_data, mappings withoutsource_output, layout algorithm changes, and manual port repositioningImportant
Acceptance Criteria
x/yposition