Tidal icon indicating copy to clipboard operation
Tidal copied to clipboard

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

Open JeffreyBenjaminBrown opened this issue 2 years ago • 3 comments

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.

JeffreyBenjaminBrown avatar Jan 03 '22 16:01 JeffreyBenjaminBrown

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 https://github.com/tidalcycles/Tidal/blob/669fde100a04ed1f86460a2f38ca618aa8d12150/test/TestUtils.hs#L34 (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 avatar Jan 05 '22 23:01 jwaldmann

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.

jwaldmann avatar Jan 06 '22 02:01 jwaldmann

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).

JeffreyBenjaminBrown avatar Jan 06 '22 03:01 JeffreyBenjaminBrown