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.
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
| 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>
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