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 Bifunctor instance for PolyDiff #2

Open
jonathanlking opened this issue Oct 31, 2022 · 16 comments
Open

Add Bifunctor instance for PolyDiff #2

jonathanlking opened this issue Oct 31, 2022 · 16 comments

Comments

@jonathanlking
Copy link

jonathanlking commented Oct 31, 2022

The following Bifunctor instance would be very handy:

instance Bifunctor PolyDiff where
  bimap f _ (First a) = First (f a)
  bimap _ g (Second b) = Second (g b)
  bimap f g (Both a b) = Both (f a) (g b)

I appreciate supporting base >= 3 && <= 6 makes things slightly more complicated, as Bifunctor is only available since base-4.8.0.0. The two options I can think of are using a CPP macro, or increasing the minimum bounds of base.

I'm happy to open a PR with either approach (or something else!).

@jonathanlking jonathanlking changed the title Add Bifinctor instance for PolyDiff Add Bifunctor instance for PolyDiff Oct 31, 2022
@ddssff
Copy link
Member

ddssff commented Oct 31, 2022

I think we can abandon base older than 4.8.0.0. If someone complains I'll welcome a PR with ifdefs.

@ddssff
Copy link
Member

ddssff commented Oct 31, 2022

Have you noticed any slowness in the diff function? I may have seen this when running in the browser via ghcjs.

@jonathanlking
Copy link
Author

Have you noticed any slowness in the diff function? I may have seen this when running in the browser via ghcjs.

We're only using the getGroupedDiff function (on GHC 8.8.4 targeting x86) and it's only used as part of tests.
I haven't noticed it being particularly slow, but that said these tests are not something that we've profiled, as they're fast enough.

@ddssff
Copy link
Member

ddssff commented Oct 9, 2023

I can do the CPP.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 9, 2023

@ddssff did you test it with GHC 9.6? I suspect instance Functor is missing.

@ddssff
Copy link
Member

ddssff commented Oct 9, 2023

I don't have access to ghc-9.6. Stuck on 8.10.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 9, 2023

Sorry, I’m AFK until the next week. If you wish, once back home, I can generate a GitHub Actions CI setup with haskell-ci covering a range of GHCs up to 9.6.

@Bodigrim
Copy link
Collaborator

@ddssff could you possibly make a release some time soon?

@ddssff
Copy link
Member

ddssff commented Oct 22, 2023

Hackage upload? Will do.

@Bodigrim
Copy link
Collaborator

Yes, please.

@ddssff
Copy link
Member

ddssff commented Oct 22, 2023

Does O(ND) in the title refer to time or space complexity?

@Bodigrim
Copy link
Collaborator

Time is O(ND) and space is O(D^2).

I'm not sure "O(ND)" is terribly helpful as a top-level package description without explaining what D refers to. It might be helpful to bring Cabal "description:" field in line with the module header:

Diff/Diff.cabal

Lines 3 to 4 in 026aef2

synopsis: O(ND) diff algorithm in haskell.
description: Implementation of the standard diff algorithm, and utilities for pretty printing.

-- This is an implementation of the diff algorithm as described in
-- \"An \( O(ND) \) Difference Algorithm and Its Variations (1986)\"
-- <http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.4.6927>.
-- For inputs of size \( O(N) \) with the number of differences \( D \)
-- it has \( O(ND) \) time and \( O(D^2) \) space complexity.

@ddssff
Copy link
Member

ddssff commented Oct 22, 2023

How about this:

synopsis:            Diff algorithm in pure Haskell
description:         Implementation of the standard diff algorithm, and utilities for pretty printing.  Time complexity is proportional to N (input length) & D (number of differences).  Space complexity is D^2.

@Bodigrim
Copy link
Collaborator

Thanks for a quick turnaround with a release, really much appreciated.

How about this:

Looks great! And today I learned that one can edit package description via revision :)

@ddssff
Copy link
Member

ddssff commented Oct 23, 2023 via email

@Bodigrim
Copy link
Collaborator

I'm thinking of removing 'getContextDiffOld' - I can't see how it could be useful to anyone.

Makes sense to me.

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