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

TreeLayout only handles TreeLayout::RootSelectionType::Source #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lypoluz
Copy link
Contributor

@lypoluz lypoluz commented Oct 19, 2023

Addresses issue #195 (314 on GitLab).
The layout may not be what one would expect but it does not crash anymore.
Adjusting the layout would require further work.

@netlify
Copy link

netlify bot commented Oct 19, 2023

Doxygen Documentation successfully built!

Name Link
🔨 Latest commit 5b712a5
🔍 Latest deploy log https://app.netlify.com/sites/ogdf/deploys/656704a86a73de0008244938
😎 Deploy Preview https://deploy-preview-102--ogdf.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lypoluz lypoluz force-pushed the dpotulski-treelayout branch 3 times, most recently from 7636605 to 449bed4 Compare October 25, 2023 12:47
@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lypoluz lypoluz marked this pull request as ready for review November 1, 2023 13:29
Copy link
Member

@milsen milsen left a comment

Choose a reason for hiding this comment

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

I tested this "manually" by setting m_selectRoot = RootSelectionType::Sink in TreeLayout::call() directly and reversing the edges of the arborescences in graphs.h via G.reverseAllEdges(). When running test-misc --only=TreeLayout, it did not crash but ran into an infinite while-loop. So to me it seems like this does not work at all, am I missing something?

The reason appears to be that the constructed TreeStructure later also assumes that the graph is always an out-arborescence. So (at least) the checks in TreeStructure would also have to be changed.


#### A new parameter 'outTree' was added to isArborescence(...) and isArborescenceForest(...).
By default it is set to true.
If set to false, root nodes are handled as sinks.
Copy link
Member

Choose a reason for hiding this comment

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

Since the default behavior is the same, and thus people do not need to change their code because of this change, we do not need to add anything to the porting guide.
(Which also solves the problem that currently, line 5 would be a lie. 😄)

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