Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

Fixes for julia v1.0 #12

Merged
merged 5 commits into from
Nov 5, 2018
Merged

Conversation

simonschoelly
Copy link
Contributor

This should get LightGraphsMatching running on Julia v0.7 and v1.0.

It does not address #10

REQUIRE Outdated Show resolved Hide resolved
export MatchingResult, maximum_weight_matching, maximum_weight_maximal_matching, minimum_weight_perfect_matching

"""
type MatchingResult{T}
weight::T
type MatchingResult{U}
Copy link
Member

Choose a reason for hiding this comment

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

Why U as type parameter?
We can also change the annotation type -> struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took some inspiration from SimpleWeightedGraphs, where this is the type of the edge-value. I think it is confusing when we use T for weights, as it is usually the vertex type in JuliaGraphs. Not sure if U is the best choice though, might be better to use W or WE or EW.

test/REQUIRE Outdated Show resolved Hide resolved
@simonschoelly
Copy link
Contributor Author

So, BlossomV.jl still fails to compile on a macintosh. This might be related to this: #mlewe/BlossomV.jl/pull/14 . BlossomV.jl runs travis only on linux, so that never got detected.
It also failed on my computer running arch linux, thats why I had to make that other pr.

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #12 into master will decrease coverage by 43.63%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #12       +/-   ##
===========================================
- Coverage     100%   56.36%   -43.64%     
===========================================
  Files           4        4               
  Lines         113      110        -3     
===========================================
- Hits          113       62       -51     
- Misses          0       48       +48
Impacted Files Coverage Δ
src/LightGraphsMatching.jl 100% <100%> (ø) ⬆️
src/lp.jl 45.09% <25%> (-54.91%) ⬇️
src/blossomv.jl 70% <33.33%> (-30%) ⬇️
src/maximum_weight_matching.jl 60.71% <62.5%> (-39.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50edcb4...a151415. Read the comment docs.

@simonschoelly
Copy link
Contributor Author

There actually might be more problems with the macintosh version: #mlewe/BlossomV.jl/issues/11

@Gnimuc
Copy link

Gnimuc commented Oct 28, 2018

coming from slack, I guess the best way to solve mlewe/BlossomV.jl#11 is to switch the building system with BB2 instead of the old BinDeps.jl.

@simonschoelly
Copy link
Contributor Author

I think we need somebody with a macintosh to fix that, because BlossomV builds fine on linux.

@matbesancon
Copy link
Member

A Mac docker image could do the job too I guess

@Gnimuc
Copy link

Gnimuc commented Oct 30, 2018

BB2 works fine: mlewe/BlossomV.jl#15, but the original code has a very disgusting license.

I would be very happy to meet those lawyers if they would bother to come to China for the code of a ten-years-old algorithm. (just kidding)

@sbromberger
Copy link

Re: the BlossomVBuilder license: we must stay away from this at all costs, regardless of the likelihood of legal consequences. If it's not a free license, it can't be in JuliaGraphs.

@coveralls
Copy link

coveralls commented Nov 2, 2018

Coverage Status

Coverage decreased (-43.6%) to 56.364% when pulling a151415 on simonschoelly:fixes-for-v1.0 into 50edcb4 on JuliaGraphs:master.

simonschoelly and others added 5 commits November 3, 2018 00:32
Co-Authored-By: simonschoelly <sischoel@gmail.com>
Co-Authored-By: simonschoelly <sischoel@gmail.com>
@simonschoelly
Copy link
Contributor Author

BlossomV now works on macs and I just rebased, so afterwards this probably can be merged.

@matbesancon matbesancon merged commit 9883489 into JuliaGraphs:master Nov 5, 2018
@matbesancon
Copy link
Member

All greed on Travis, merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants