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

Fix always existing bug in model order port sorting. #1067

Merged

Conversation

soerendomroes
Copy link
Contributor

@soerendomroes soerendomroes commented Aug 15, 2024

Make sure that the order of incoming ports matches the order given by the previous layer.

Updated WEST side handling to fix the main issue. Moreover, there was a p1/p2 typo as well.

This also makes port model order non-constraining for input ports same as the edge model order.
If one wants to constrain the port order one should use portConstraints.

  • I want to check whether I can also fix feedback edges with model order
  • Check the used examples into elk-models to document how it should behave.

the previous layer.

Updated WEST side handling to fix this and fixed typo as well and makes
port model order non constraining for input ports.
@soerendomroes soerendomroes added this to the Release 0.9.2 milestone Aug 15, 2024
@soerendomroes
Copy link
Contributor Author

soerendomroes commented Aug 15, 2024

This shows the model I wanted to fix and the one I broke.

Apparently my bug only works for models were the edges originate from the same node.

@soerendomroes
Copy link
Contributor Author

This does not seem to be everything. Somehow, this is different.

@soerendomroes
Copy link
Contributor Author

eclipse/elk-models#22 adds the correctness test as an example model.

Maybe check, whether I should add more concrete tests for all corner cases here.
The test models in tests/layered/preserveOrder check all other cases while the basic correctness tests that check that big models have no corner cases left.

Copy link
Contributor

@Eddykasp Eddykasp left a comment

Choose a reason for hiding this comment

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

fix unconnected ports placement

Copy link
Contributor

@Eddykasp Eddykasp left a comment

Choose a reason for hiding this comment

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

lgtm

@soerendomroes
Copy link
Contributor Author

Fixes #1065

@soerendomroes soerendomroes merged commit 6629d35 into eclipse:master Aug 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants