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

Stackage #27

Open
fakedrake opened this issue May 31, 2020 · 16 comments
Open

Stackage #27

fakedrake opened this issue May 31, 2020 · 16 comments

Comments

@fakedrake
Copy link

Could we get a package out of this? I like your code on arrows

@svenkeidel
Copy link
Owner

svenkeidel commented Jun 2, 2020

Thanks for checking out my library. Yes, this sounds like a good idea. @seba--, what do you think?

I'm trying to decide if I should split this library into smaller packages or if I should upload it as a whole. @fakedrake, which part of the library would you use? Would you use the library for implementing static analyses or would you only use the arrow code for implementing other applications?

@fakedrake
Copy link
Author

The overall issue I am facing is that there is no arrow transformer library on stackage so I have to re-implement everything. Which sucks. So a package with everything under Control.Arrow would be perfect.

@svenkeidel
Copy link
Owner

svenkeidel commented Jun 2, 2020

Makes sense. Well, there is the arrow transformer library atl. Have you tried atl and is there a problem with it?

Furthermore, currently half of whats in sturdy:Control.Arrow is specific to static analyses and this is probably of no use for the day to day developer. What would be of use are the general purpose transformers ReaderT, WriterT, StateT, ConstT, StaticT, ContT, KleisliT, and CoKleisliT. These transformers could be extract into their own package, however, I need to be convinced that this is worth the effort. Maybe it is worth to contribute some of this code to the atl project.

@svenkeidel
Copy link
Owner

svenkeidel commented Jun 2, 2020

Here is a list of all the files that are general purpose and reusable. Now you just need to remove missing imports and type class instances. I removed the ExceptT-like transformers because they include some details specific to static analysis.

@fakedrake
Copy link
Author

The list looks legit. TBH I only shamelessly copied KleisliT, StateT, ReaderT and ContT (so far). Better than atl is arrows but it's not on stackage, it seems discontinued, it's missing important transformers (ehm KleisliT), and most importantly I like the implementations in sturdy much better.

@svenkeidel
Copy link
Owner

svenkeidel commented Jun 3, 2020

Thanks. We put quite some work into the arrow code to improve the generated code and its performance.

Ok, maybe its time for a new arrow library. I see what I can do.

@seba--
Copy link
Collaborator

seba-- commented Jun 3, 2020

I think this is a great idea.

@svenkeidel
Copy link
Owner

@svenkeidel
Copy link
Owner

svenkeidel commented Jun 3, 2020

Allright, I extracted the library and added some minimal documentation. @fakedrake, can you give me some feedback on the library in the arrows folder please? In particular:

  • Is documentation missing/confusing?
  • Are any of the type class and function names confusing? Are there any type class methods missing?

I would greatly appreciate any contributions in terms of feedback, documentation and code. Thanks.

@fakedrake
Copy link
Author

Wow you did quite a bit of work, thanks! Anyway documentation looks legit. Thanks for moving some benchmarks in there too.

What's up with changing around ArrowTrans and ArrowLift?

@fakedrake
Copy link
Author

This is low priority by I like Automaton, if you ever get the chance to add it. This implementation has better naming and implementation (IMHO) but it's not a transformer.

@svenkeidel
Copy link
Owner

What's up with changing around ArrowTrans and ArrowLift ?

I swapped the names to make the name of ArrowTrans more consistent with mtl's MonadTrans. The ArrowTrans.lift' function is the arrow equivalent of MonadTrans.lift.

This is low priority by I like Automaton

Sure I see what I can do. I have never used this transformer. What is it useful for?

@fakedrake
Copy link
Author

One example might be circuits. I have seen in for stream processing but I can't find the citation right now.

@fakedrake
Copy link
Author

fakedrake commented Jun 5, 2020

I saw you added it, thanks a lot! Final request and I will leave you alone: could you move FailureT in there as well?
Edit: FailureT inherits everything from KleisliT but it doesn't really need ArrowMonad for ... well everything, just ArrowChoice. I will write it up when I get the chance but here is a very rough sketch of what I propose:

instance (ArrowChoice c) => Category (KleisliT Maybe c) where
  KleisliT f . KleisliT g = KleisliT $ g
    >>> arr (maybe (Left ()) Right)
    >>> (arr $ const Nothing) ||| f
  id = KleisliT $ arr return . id

instance (Category (KleisliT f c),Arrow c,Monad f) => Arrow (KleisliT f c) where
  arr = arrLift . arr
  first (KleisliT f) = KleisliT $ first f >>> arr (\(fc,u) -> (,u) <$> fc)

@svenkeidel
Copy link
Owner

I see. So you want to eliminate the ArrowMonad class and instead have a Category Kleisli instance vor every Monad. I like this idea.

About adding FailureT: the only problem I have is that the ArrowFail class is too general due to it's use for static analysis. Either we use this more general version or introduce a simpler version without the Join constraint.

@fakedrake
Copy link
Author

Sorry, I am realizing that what I wrote was not clear at all. What I mean is this:

newtype ExceptT e c a b = ExceptT (c a (Either e b))
instance ArrowChoice c => Category (ExceptT e c) where
  ExceptT f . ExceptT g = ExceptT $ g >>> arr Left ||| f
  id = ExceptT $ arr return . id

instance ArrowChoice c => Arrow (ExceptT f c) where
  arr f = ExceptT $ arr $ return . f
  first (ExceptT f) = ExceptT $ first f >>> arr (\(fc,u) -> (,u) <$> fc)

The problem is that as far as I can tell, the only ArrowMonad is KleisliT and the arrows implemented as such, but almost all the arrows in the package are ArrowChoice.

I know performance is important for sturdy so disregard this if this change affects performance badly.

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

No branches or pull requests

3 participants