Skip to content

Some fixes for v1.0 #66

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

Merged
merged 8 commits into from
Sep 27, 2018
Merged

Conversation

simonschoelly
Copy link
Member

See #65
I fixed some things to make this work on Julia 1.0 . Unfortunately Compose.jl is not yet ready and produces a stack overflow when trying to print a Compose.Context to the console and there also seem to be some other problems when running the tests for GraphPlot.jl.

What doesn't work:

using LightGraphs, GraphPlot
g = CompleteGraph(3)
gplot(g)

what works:

using LightGraphs, GraphPlot, Cairo
g = CompleteGraph(3)
draw(PDF("test.pdf", 16cm,16cm), gplot(g))

and

using LightGraphs, GraphPlot
g = CompleteGraph(3)
gplothtml(g)

So this also seems to fix #54

Unfortunately, I also had to add Arpack.jl to the requirements. And I took the liberty of bumping the version requirement to 0.7.

@simonschoelly
Copy link
Member Author

Strangely, Github somehow shows a commit for a different pr (from a different branch) in the commit tab for this pr.

@simonschoelly
Copy link
Member Author

The problems with Compose are now fixed and we just have to wait for them to tag a new version. In the meantime, adding Compose#master should make this pr already work, for those who need it.

There was a strange problem on my computer. Apparently it is not enough to specify VisualRegressionTests 0.2.2 in tests/REQUIRE but it also has to be specified in REQUIRE, even though it is only used for testing. We will have to check if this is also the case on travis or if this is just a problem with my computer.

REQUIRE Outdated
@@ -1,4 +1,5 @@
julia 0.6
julia 0.7
Arpack
Copy link
Member

Choose a reason for hiding this comment

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

do we need this? Can we use the [email protected] LightGraphs.LinAlg.eigs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need that. I therefore changes the code so that it uses LightGraphs.LinAlg.eigs.

src/layout.jl Outdated
@@ -259,7 +261,7 @@ function _spectral(A::SparseMatrixCSC)
D = sparse([1:length(data)], [1:length(data)], data)
L = D - A
ncv = max(7, int(sqrt(length(data))))
eigenvalues, eigenvectors = eigs(L, nev=3, ncv=ncv)
eigenvalues, eigenvectors = LightGraphs.LinAlg.eigs(L, nev=3, ncv=ncv)
Copy link
Member

Choose a reason for hiding this comment

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

you will need to specify the which=LR() for this eigs call.
and using ArnoldiMethods in order for this to work

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, I though I thought LightGraphs.LinAlg.eigs was just an identical replacement for the old eigs functions. Any reason why I should use LightGraphs.LinAlg.eigs instead of partialschur directly?

@simonschoelly
Copy link
Member Author

Apparently not all lines of the code are covered by tests and therefore there were a lot of errors that slipped by me. The situation should be better now, but we need more tests.
See #67

@simonschoelly
Copy link
Member Author

Compose has tagged a new version. There where some strange errors, that prevented the tests from passing on Travis, but I found some solutions for them and made some notes in the comments.

@sbromberger sbromberger merged commit 726aec6 into JuliaGraphs:master Sep 27, 2018
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.

gplothtml doesn't work
3 participants