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

Make iterations in SugiyamaAlgorithm configurable #57

Merged
merged 8 commits into from
Nov 24, 2021

Conversation

JulianBissekkou
Copy link

No description provided.

@JulianBissekkou
Copy link
Author

@nabil6391

This PR introduces some smaller changes that help us increase performance. The first one is making the iterations in nodeOrdering configurable since for all our graphs one iteration is enough. This change is non breaking since i am keeping the previous value of 10.

I also avoided recalculating some stuff and did some smaller optimizations in crossingb which is called very often. The increased performance just a little bit. As you said the bigger fix would be to optimize assignX and transpose.
This is still open. See #56 .

I also did some smaller code style changes, but i can revert them if you like.

Thanks for taking the time!

@@ -863,7 +858,7 @@ class SugiyamaAlgorithm extends Algorithm {
nodeOrdering(); //expensive operation
coordinateAssignment(); //expensive operation
// shiftCoordinates(shiftX, shiftY);
final graphSize = calculateGraphSize(this.graph);
//final graphSize = calculateGraphSize(this.graph);
Copy link
Author

Choose a reason for hiding this comment

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

This was unused so i commented it out 👍🏽

@nabil6391
Copy link
Owner

Thanks for this. I will create some tests and have a look at this on the weekend

@nabil6391
Copy link
Owner

Hi @JulianBissekkou , I have added two tests in the repo, can you please try to run them and see if they pass. Thanks

Other than that LGTM

@JulianBissekkou
Copy link
Author

@nabil6391 Tests are passing! I also created a workflow file that runs tests in master and on every pr. I think you have to enable github actions in the project otherwise I can not test if the workflow works :)

@nabil6391 nabil6391 merged commit c693878 into nabil6391:master Nov 24, 2021
This pull request was closed.
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