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

Add induceJust #209

Merged
merged 7 commits into from
Jun 7, 2019
Merged

Add induceJust #209

merged 7 commits into from
Jun 7, 2019

Conversation

Avasil
Copy link
Contributor

@Avasil Avasil commented Jun 4, 2019

Fixes #202

src/Algebra/Graph.hs Outdated Show resolved Hide resolved
src/Algebra/Graph.hs Outdated Show resolved Hide resolved
src/Algebra/Graph/AdjacencyMap.hs Outdated Show resolved Hide resolved
test/Algebra/Graph/Test/Generic.hs Show resolved Hide resolved
@snowleopard
Copy link
Owner

@Avasil Looks great, many thanks! I've added a few minor comments.

@snowleopard
Copy link
Owner

By the way, we could also add induceJust1 to NonEmpty.* modules, which has a cute type signature:

induceJust1 :: Graph (Maybe a) -> Maybe (Graph a)

Feel free to ignore this if you don't have enough time.

@Avasil
Copy link
Contributor Author

Avasil commented Jun 5, 2019

@snowleopard Thanks for the review! I implemented NonEmpty versions and other comments
It seems like the build is failing now https://api.travis-ci.org/v3/job/541821370/log.txt

Mostly in RewriteRules - is it nondeterministic test, or could it be because of my changes?

@snowleopard
Copy link
Owner

Hmm, I'm not sure what's going on with rewrite rules. @nobrakal Could you please have a look?

@nobrakal
Copy link
Contributor

nobrakal commented Jun 5, 2019

That is really strange, I just ran the test suite, and it compiled normally on my machine (with stack test). The failing from Travis seems pretty strange too... I will investigate further.

Speaking of rewrite rules, at first glance, it seems that induceJust could benefit from the same kind of rule than for induce (namely the "buildR/induce" rule). Even if this is a not-so-beautiful rule, it feels strange to have one for induce and not for induceJust.

Ps: I just noticed that the bench suite was broken (again...), I really need to fix this once and for all !

@snowleopard
Copy link
Owner

I'll try to fix the CI here: #210

@snowleopard
Copy link
Owner

@Avasil Would you like to be listed in https://github.com/snowleopard/alga/blob/master/AUTHORS.md? If yes, please add yourself to the list.

algebraic-graphs.cabal Outdated Show resolved Hide resolved
@Avasil
Copy link
Contributor Author

Avasil commented Jun 6, 2019

@snowleopard Thanks, I added myself!

@nobrakal Good idea, I can look into that but I'd rather do it in separate PR

@snowleopard
Copy link
Owner

@Avasil Great! All looks good now. I'll merge once CI passes.

@snowleopard snowleopard merged commit 99fd4f8 into snowleopard:master Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add induceJust
3 participants