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

Revise documentation and tests for induceJust #211

Merged
merged 2 commits into from Jun 7, 2019
Merged

Conversation

snowleopard
Copy link
Owner

See #202 and #209.

@snowleopard
Copy link
Owner Author

@nobrakal @Avasil Note that we now have these properties in docs/tests:

induceJust  . fmap (\x -> if p x then Just x else Nothing) == induce  p
induceJust1 . fmap (\x -> if p x then Just x else Nothing) == induce1 p

It would be nice to check if we could use this in the implementations of induce and induce1 of algebraic graph data types without any performance loss, thanks to rewrite rules :)

@snowleopard snowleopard merged commit 98541ae into master Jun 7, 2019
@snowleopard snowleopard deleted the minor-fixes branch June 7, 2019 20:11
@Avasil
Copy link
Contributor

Avasil commented Jul 10, 2019

@snowleopard
I will have some time to contribute to alga and it would be cool to check out those rewrite rules. :)

I have a question about benchmarking though. Do you have any example how to do it properly? I know about https://github.com/haskell-perf/graphs but it looks like it loads a dependency so I can't easily check different functions' implementations

@snowleopard
Copy link
Owner Author

@Avasil Great! :)

@nobrakal What is the right way to use your benchmarking suite when developing Alga? Perhaps, the performance suite that we run on Travis on every commit is a good place to start, but unfortunately, it seems to be still broken :(

@nobrakal
Copy link
Contributor

@snowleopard I fixed (again) the bench suite, it should run. Anyway, I am not really proud of the code behind it as I tried to reinvent the wheel while I should have used more primitive from criterion. It is on my todo list to rewrite the backend ^^.

@Avasil bench-graph was not really made to be used while developing Alga, so I made a (very ugly) shell script to automatically test a local version of the alga repostiory against master: https://github.com/nobrakal/benchAlgaPr/blob/master/compare.sh (it is the one used in Travis).

The help message should be informative.

Just if you want to mess with it, it actually makes a copy of alga's master branch, changes its name to "old", adds it as a library in bench-graph, and launch the benchmarking suite against the given alga repository.

@snowleopard
Copy link
Owner Author

@nobrakal Many thanks for the fix! :-)

@snowleopard
Copy link
Owner Author

@nobrakal Actually, something is wrong.

Could you look at this failure? https://travis-ci.org/snowleopard/alga/jobs/557899321.

@nobrakal
Copy link
Contributor

Ah, I forgot to mention that the suite needs many dependencies, and because I updated them, Cabal re-downloaded them all.

@snowleopard Just restart the build, the cache should've been updated now.

@snowleopard
Copy link
Owner Author

@nobrakal Indeed, green now, thanks!

@Avasil
Copy link
Contributor

Avasil commented Jul 14, 2019

@nobrakal Thanks for the script, I will try it soon!

If you don't mind, I have a question about rewrite rules. I'm also adding rewrite rule to induceJust as you suggested. My first try is:

"buildR/induceJust" [~1] forall g.
    induceJust g = buildR (\e v o c -> foldg e (maybe e v) o c g)

I followed buildR/induce:

"buildR/induce" [~1] forall p g.
    induce p g = buildR (\e v o c -> foldg e (matchR e v p) o c g)

Though I had to use maybe. I don't yet understand how rewrite rules works but I assume it could break optimization? Can maybe be defined in similar way to matchR?

@nobrakal
Copy link
Contributor

I realized I did not explain what was going on here.

Let me sum up:

When expressed with buildR, a function will be optimized if composed later with foldg (using the "foldg/buildR" rule).

induce is not a "typical" function, because it removes empty leaves. In its buildR form (from "buildR/induce"), this behaviour is not kept (because it seems impossible). So the idea is, if the rewriting did not happen, to "turn things back", using the "graph/induce" rule.

The only problem is that we want to recognize the form of the right part of the equality of "buildR/induce" when the optimization did not happen. So we define and use matchR, that will not be inlined too soon, and we can target it with "graph/induce".

For your case, I think you should define something like:

maybeR :: b -> (a -> b) -> Maybe a -> b 
maybeR e f x = maybe e f x
{-# INLINE [0] maybeR #-}

and follow the same process.

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

3 participants