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

Define the Eq and Ord instances for the Event type by hand. #890

Open
wants to merge 1 commit into
base: 2.0-beatmode-retired
Choose a base branch
from

Conversation

JeffreyBenjaminBrown
Copy link
Contributor

This way, otherwise-equal Events are considered equal
even if their Contexts (debugging information) differ.
Also, Events are compared first on when they happen
(the part field), second on their value,
and only last on their whole field (which is often Nothing).
Sorting a List of Events will thus cause the early ones
to be first in the List, and contemporaneous ones
to be adjacent in the list even if their wholes differ.

  This way, otherwise-equal `Event`s are considered equal
  even if their `Context`s (debugging information) differ.
  Also, `Event`s are compared first on when they happen
  (the `part` field), second on their `value`,
  and only last on their `whole` field (which is often `Nothing`).
  Sorting a `List` of `Event`s will thus cause the early ones
  to be first in the `List`, and contemporaneous ones
  to be adjacent in the list even if their `whole`s differ.
@jwaldmann
Copy link
Contributor

jwaldmann commented Jan 5, 2022

I feel a bit uneasy about this. The other suggested change (remove Context from instance Show Event) is just for human-readable output, but this here concerns == (etc.) that may be important for code. Are we sure that actual (derived) equality is never used (now, and in the future)?

The main motivation is for writing tests? There are are already some classes/functions for
comparing (sets of) events, defined in

instance TolerantEq (Event ValueMap) where
(TolerantEq, compareP, comparePD). How does this relate?

If there are some Events where Context is important, and some where it is not, then
the True Haskell Way is to make them distinct types, so they cannot be confused,
and each can have their specific class instance(s). Or

data EventF c a b = EventF { context :: c, part :: a, value :: b, ... }

The function you wrote

relevant e = (whole e, part e, value e)

actually wants to have type EventF c a b -> EventF () a b.

An intermediate step (that does not break anything) would be to just
hoist it to top level, and export it, perhaps as

withoutContext :: Event a -> Event a
withoutContext e = e { context = [] }

@jwaldmann
Copy link
Contributor

Re: are we sure that derived Eq is never used

I commented out the deriving (Eq, Ord) clause, this creates an error in defragParts because of

 src/Sound/Tidal/Pattern.hs:586:57: error:
    • Could not deduce (Eq (Event a)) arising from a use of ‘delete’

in this code

defragParts (e:es) | isJust i = defraged : defragParts (delete e' es)
       ...
  where i = findIndex (isAdjacent e) es
        e' = es !! fromJust i

Here, delete is unnecessary (so we don't need instance Eq (Event a))
since we already know the index of the element to be removed, so a function like deleteAt https://hackage.haskell.org/package/containers-0.6.5.1/docs/Data-Sequence.html#v:deleteAt
should be used (but it seems Data.List does not have it)

Then, the Eq and Ord instances are used in

compareDefrag :: (Ord a) => [Event a] -> [Event a] -> Bool
compareDefrag as bs = sort (defragParts as) == sort (defragParts bs)

Is this supposed to respect Contexts, or to ignore them? The only application I found is in
comparePD (used in tests)

comparePD a p p' = compareDefrag es es'
  where es = query (stripContext p) (State a Map.empty)
        es' = query (stripContext p') (State a Map.empty)

A-ha, the proposed context-removal function did exist all along. Well, for patterns it did.

stripContext = setContext $ Context []
setContext c pat = withEvents (map (\e -> e {context = c}))

Let's give a name (withoutContext, or similar) to this anonymous function here.

@JeffreyBenjaminBrown
Copy link
Contributor Author

stripContext is certainly a handy thing to be able to do.

I wouldn't be terribly bothered if the changes to Eq didn't make it through. In fact even keeping context part of the Ord instance could be acceptable -- if it were considered last among fields, rather than first (as it is now).

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.

2 participants