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

layered: Added model order cycle breaker. #759

Merged
merged 11 commits into from Sep 15, 2021

Conversation

soerendomroes
Copy link
Contributor

@soerendomroes soerendomroes commented Jun 21, 2021

Added a cycle breaker that reverses edges going against the model order.
Edges should always go from low model order to high model order.

  • Using this strategy in a model with layer constraints causes an NPE. One could either ignore these layer constraints and only use the model order or use it additionally to the model order (which might be the better solution).
  • Check whether hierarchical and normal graphs cover this cycle breaker via tests

Signed-off-by: Soeren Domroes sdo@informatik.uni-kiel.de

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@soerendomroes
Copy link
Contributor Author

@uruuru Any idea why elkjs is not working? Something seems to use Java 11 instead of Java 8.

@soerendomroes soerendomroes self-assigned this Jun 29, 2021
correct exception, fixed copy-paste bug.

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Copy link
Contributor

@uruuru uruuru left a comment

Choose a reason for hiding this comment

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

Using this strategy in a model with layer constraints causes an NPE. One could either ignore these layer constraints and only use the model order or use it additionally to the model order (which might be the better solution).

What happens if the constraints are added programatically, for instance, for hierarchical ports? What is the model index of a hierarchical port's dummy node in the first place? Did you check with hierarchical graphs?

Could you please also check if there are unit tests where you'd have to manually add this strategy? Or are existing tests succeeding?

Also removes author tag and check for self loops.

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@soerendomroes
Copy link
Contributor Author

soerendomroes commented Aug 24, 2021

Somehow the ModelOrderCycleBreaker seems to create graphs that cannot be handled by the SortByInputModelOrderProcessor. Using only one of them does not cause problems.

Solved by handling saving the transitive dependencies and using them instead.

transitive.

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
sdo/modelOrderCycleBreaker

Conflicts:
	plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/graph/transform/ElkGraphImporter.java
	plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/intermediate/preserveorder/ModelOrderNodeComparator.java

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@soerendomroes
Copy link
Contributor Author

soerendomroes commented Sep 2, 2021

What happens if the constraints are added programmatically, for instance, for hierarchical ports? What is the model index of a hierarchical port's dummy node in the first place? Did you check with hierarchical graphs?

Checked everything, programmatic constraints are handled now.

Could you please also check if there are unit tests where you'd have to manually add this strategy? Or are existing tests succeeding?

I added this cycle breaker to the BasicCycleBreakerTest

@soerendomroes soerendomroes merged commit 5f4ffc1 into eclipse:master Sep 15, 2021
@soerendomroes soerendomroes added this to the Release 0.8.0 milestone Mar 7, 2022
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.

None yet

2 participants