Visitar URL original
Add definitions for `stimes` by konsumlamm · Pull Request #340 · haskell-unordered-containers/unordered-containers · GitHub
Skip to content

Add definitions for stimes - #340

Merged
sjakobi merged 2 commits into
haskell-unordered-containers:masterfrom
konsumlamm:semigroup-monoid
Jan 6, 2022
Merged

sjakobi merged 2 commits into
haskell-unordered-containers:masterfrom
konsumlamm:semigroup-monoid

Conversation

@konsumlamm

@konsumlamm konsumlamm commented Dec 19, 2021 •

Copy link
Copy Markdown
Contributor

Resolves part of #307.

I added INLINE pragmas, for consistency with the other definitions, although they're probably not needed.

Also removed two unused {-# LANGUAGE LambdaCase #-}.

EDIT: I originally also added definitions for mconcat and sconcat, but removed them again (see discussion below).

Remove unused `LambdaCase` extension

@sjakobi sjakobi left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

The stimes definitions look good, but I'm less sure with sconcat and mconcat.

For these I think it would be best to have some tests to check the Semigroup and Monoid laws. We can probably use checkers or quickcheck-classes for this. (The transitive semigroupoids dependency may need to be configured with -f-unordered-containers.)

The performance implications also are unclear to me. I think it would be best to resolve #139 before using unions in mconcat and sconcat.

Do you want to work on this? If you don't (which would be perfectly fine!), I suggest you remove the changes to mconcat and sconcat for now.

@treeowl

treeowl commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

@sjakobi it all looks fine to me. I agree tests would be good (for stimes as well).

@sjakobi

sjakobi commented Dec 19, 2021

Copy link
Copy Markdown
Member

@treeowl What do you think about #139? Please do chime in on the discussion there!

@konsumlamm

Copy link
Copy Markdown
Contributor Author

For these I think it would be best to have some tests to check the Semigroup and Monoid laws.

Are you referring to the mconcat = foldr (<>) mempty law? This would be broken with my implementation, although other packages (e.g. containers) don't respect this law either, so idk how important this is for you.

The performance implications also are unclear to me. I think it would be best to resolve #139 before using unions in mconcat and sconcat.

Do you want to work on this? If you don't (which would be perfectly fine!), I suggest you remove the changes to mconcat and sconcat for now.

Good point, I'll remove the mconcat/sconcat definitions (#139 seems to be non-trivial to solve).

@treeowl

treeowl commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

I'm just saying that we have to be careful not to change the current semantics (whatever they are) unintentionally.

@konsumlamm konsumlamm changed the title Add definitions for mconcat, sconcat, stimes Add definitions for stimes Dec 19, 2021
@sjakobi

sjakobi commented Dec 19, 2021

Copy link
Copy Markdown
Member

For these I think it would be best to have some tests to check the Semigroup and Monoid laws.

Are you referring to the mconcat = foldr (<>) mempty law? This would be broken with my implementation, although other packages (e.g. containers) don't respect this law either, so idk how important this is for you.

I was referring to that law and the other ones documented in base. How does containers break the law BTW? Please make a bug report!

@sjakobi

sjakobi commented Dec 19, 2021 •

Copy link
Copy Markdown
Member

So the semantic change resulting from the explicit stimes definitions is that we now have

stimes 0 x = mempty

instead of before

stimes 0 x = undefined

I'd be comfortable with including this in a minor release. containers did the same with Seq in v0.6.1.1 (haskell/containers@c9d1e69). What do you think, @treeowl?

@treeowl

treeowl commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

Yeah, a semantic improvement is totally fine. I'm talking about the left vs. right bias in sconcat/mconcat. If we change it, we need to document, make a major version, etc.

@konsumlamm

Copy link
Copy Markdown
Contributor Author

How does containers break the law BTW? Please make a bug report!

I was wrong, it doesn't.

If we just define mconcat = unions (with the current definition of unions), that should have the same behaviour (since union is associative). However, if we decide to define unions = foldl' (flip union) empty (which is what was originally suggested in #139), that wouldn't be the case anymore (although that would also be a breaking change for unions).

@treeowl

treeowl commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

@konsumlamm I imagine that we could define a unionOtherWay that's structured like union but prefers the other map. It would be interesting to see if a flipped version of that did a better job than the current unions.

@sjakobi
sjakobi merged commit 6547038 into haskell-unordered-containers:master Jan 6, 2022
@sjakobi

sjakobi commented Jan 6, 2022

Copy link
Copy Markdown
Member

Thank you, @konsumlamm! :)

@konsumlamm
konsumlamm deleted the semigroup-monoid branch January 6, 2022 13:12
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.

3 participants