`eq` comparison on time signatures #1457

MarkGotham · 2022-10-10T19:02:58Z

No description provided.

MarkGotham · 2022-10-10T19:28:42Z

... I've reviewed the attrs for ratioString and beatSequence and am pretty sure that:

that's a good basis for this comparison
these tests should work ...

mscuthbert

Some changes requested -- the part about checking class is very important.

music21/meter/base.py

mscuthbert · 2022-10-10T22:21:30Z

But see also discussion on the 1454 first.

Notably, this means removing time signatures for now while they're problematic. See cuthbertLab#1457

cuthbertLab#1466 introduces a new `__eq__` syntax and highlights some failing tests on Time Signature as 'work in progress'. This commit implements that syntax and fixes (uncomments) those tests. Possible remaining questions: - Do we want `assertRaises` on the comparison of different classes or is False good (as here)? - all and only the right checks in `core`?

MarkGotham · 2022-10-19T12:20:31Z

Latest:

implements new __eq__ syntax Equality changes -- hierarchy #1466
fixes (uncomments) the failing tests highlighted in Equality changes -- hierarchy #1466.
replaces beatSequence check (does not catch fast vs slow 6/8) with beatCount and ratioString

Possible remaining questions:

Do we want assertRaises on the comparison of different classes or is False good (as here)?
Basic __eq__ method in core seems to have been the missing link. Now fixed. Q: all and only the right checks there or something else needed?
mypy: remove bool from def __eq__(self, other) -> bool:? On both base and core? (Why does this fail on one and not the other?)

coveralls · 2022-10-19T12:23:44Z

Coverage: 93.055% (+0.002%) from 93.053% when pulling c3958a0 on MarkGotham:TimeSig__eq__ into 9afb3bc on cuthbertLab:master.

... with thanks to coveralls ;)

music21/meter/base.py

mscuthbert · 2022-10-26T19:37:38Z

music21/meter/core.py

+    def __eq__(self, other) -> bool:
+
+        if not super().__eq__(other):
+            return False
+
+        return True
+


This is also not needed. If you're just returning the value of a super() call, then there's no point in subclassing.

Ok thanks. True in so far as the actual test is not needed ... but the method is not expendable. Something is needed here, perhaps just to give MeterTerminal some notion of equality. FWIW empty equalityAttributes tuple (equalityAttributes = ()) doesn't work.

MarkGotham · 2022-11-01T11:47:02Z

Latest: the 3x equality attributes seem to cover all and only the right comparisons:

beatCount: slow 6/8 and fast 6/8 successfully treated as unequal
symbol: Cut vs 2/2
ratioString (which actually stands in for much more than just a string): 2+3 != 3+2.

See tests at the end of meter/tests for coverage and proof of this particular pudding ;)

mscuthbert · 2022-11-02T04:24:33Z

music21/meter/core.py

    def __eq__(self, other) -> bool:
-
-        if not super().__eq__(other):
-            return False
-
        return True


I still don't understand this -- what does this help in the system? I don't know of any object that always compares True to every other object.

Hi @mscuthbert .

Thanks for this. As I say, this odd little method is just a placeholder - not the final solution, but a demo/reminder that something is needed on this class and none of the following are acceptable alternatives:

def __eq__ with a simple pass

no def __eq__ but with equalityAttributes = () on the class instead

neither of those two.

Excuse the exploratory approach but I don't know why this behaves as it does - whether it's to do the new wider handling of eq you've started implementing, or something on meter.

In short, I'm just reporting on the behaviour I see for this case and what 'works' (i.e., what apparently is needed to get the musical tests returning the correct bools).

My hope is that having this oddity and discussion point helps us diagnose what's going on and replace it with something more plausible. Likely it will feed into your wider re-working of meter.

The rest seems to work well ... and without oddities.

Thanks for any ideas for a fix. Equally, just say if you want to drop this PR, e.g., if it's not a good time for it / doesn't fit with your root-and-branch reform of the meter trees ... pun intended ;)

I think it's best to comment it out. I've never heard of an object where if OBJ == True is the same as if OBJ == False and if OBJ == 5 and if OBJ == 2 are all True. It leads to huge issues with using the obj in a hash situation.

mscuthbert · 2023-01-30T08:58:56Z

Hi @MarkGotham -- just a ping on this.

This test pre-dates this PR and turns out to be out of scope.

MarkGotham · 2023-01-30T14:14:58Z

Hi @mscuthbert , @jacobtylerwalls , all

Thanks for the nudge.

Eureka!

Coming back to this fresh, I realise that the much-discussed and not resolved placeholder __eq__ method was a red herring and the issue was with a single test case that a pre-existing "Work in Progress" test that's out of scope. Specifically:

test case pre-existing this PR (was commented out as marked as WIP)
I thought it could now be included, so removed the comment marks
but actually I can't because it tests on a different class (MeterSequence).

BTW this is one of two such test case (pre-existing this PR, commented out, marked as WIP) -- the other one is on TimeSignature objects, is now included, and works fine.

So, in summary:

This PR delivers on its promise: __eq__ comparison on time signatures
We could consider a separate PR for __eq__ comparison on MeterSequence
If you're content with this then I can probably also finalise (restore time signatures to) Add utility for removing duplicated objects (e.g. keys, clefs) #1454.

mscuthbert

Looks good now. Thanks for groking the equityAttributes -- this PR really inspired making all that finally work. Will merge when the little edit passes

MarkGotham added 3 commits October 10, 2022 20:02

__eq__ comparison on time signatures

3834aa6

error (unreachable)

840217e

whitespace

84c22a1

MarkGotham mentioned this pull request Oct 10, 2022

Add utility for removing duplicated objects (e.g. keys, clefs) #1454

Merged

MarkGotham marked this pull request as draft October 10, 2022 19:28

mypy: no typing on __eq__

85b4ba6

mscuthbert requested changes Oct 10, 2022

View reviewed changes

music21/meter/base.py Outdated Show resolved Hide resolved

music21/meter/base.py Outdated Show resolved Hide resolved

music21/meter/base.py Outdated Show resolved Hide resolved

music21/meter/base.py Outdated Show resolved Hide resolved

music21/meter/base.py Outdated Show resolved Hide resolved

mscuthbert mentioned this pull request Oct 10, 2022

v9 -- equality hierarchy #1458

Closed

MarkGotham added 7 commits October 11, 2022 11:43

First, no changes to what - only where

7366496

type check

b5fa21b

remove nested comparisons

e6bddcf

remove unused (different type checking)

05928f3

move complementary test

9f82f83

clean/clarify current failing tests

21d7099

add more failing tests ;)

fb2108e

MarkGotham added a commit to MarkGotham/music21 that referenced this pull request Oct 11, 2022

explicitly limit to key.KeySignature and clef.Clef

afacdb0

Notably, this means removing time signatures for now while they're problematic. See cuthbertLab#1457

MarkGotham added 4 commits October 17, 2022 14:33

Merge branch 'master' into TimeSig__eq__

b4e8460

Merge branch 'master' into TimeSig__eq__

a8176ba

Merge branch 'master' into TimeSig__eq__

8f04a4f

right 3 attrs exactly once each

9af40b8

... with thanks to coveralls ;)

MarkGotham requested a review from mscuthbert October 19, 2022 12:32

MarkGotham marked this pull request as ready for review October 19, 2022 14:05

jacobtylerwalls reviewed Oct 20, 2022

View reviewed changes

music21/meter/base.py Outdated Show resolved Hide resolved

mscuthbert reviewed Oct 26, 2022

View reviewed changes

music21/meter/base.py Outdated Show resolved Hide resolved

mscuthbert reviewed Oct 26, 2022

View reviewed changes

MarkGotham added 4 commits November 1, 2022 11:27

remove base eq (only need equalityAttributes)

a55ef09

placeholder core eq (need something here ...)

b08de8e

comment motivation for each equalityAttribute

5b9b711

Sphinx fix

b3d395e

mscuthbert reviewed Nov 2, 2022

View reviewed changes

MarkGotham added 4 commits January 30, 2023 12:58

Merge branch 'master' into TimeSig__eq__

c1a76d9

Remove the much-discussed placeholder

a87a0b1

return one of pre-existing "Work in progress" tests to commented out

fd7fb82

This test pre-dates this PR and turns out to be out of scope.

doc tests: clarify & include complementary ratioEqual case

f02af54

MarkGotham requested a review from jacobtylerwalls January 30, 2023 14:15

Remove duplicated line, update comment

c3958a0

mscuthbert approved these changes Jan 30, 2023

View reviewed changes

mscuthbert merged commit a8ba6e4 into cuthbertLab:master Feb 1, 2023

MarkGotham added a commit to MarkGotham/music21 that referenced this pull request Feb 1, 2023

Reinstate meter.TimeSignature checks (cf cuthbertLab#1457 )

c96a580

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`eq` comparison on time signatures #1457

`eq` comparison on time signatures #1457

MarkGotham commented Oct 10, 2022

MarkGotham commented Oct 10, 2022

mscuthbert left a comment

mscuthbert commented Oct 10, 2022

MarkGotham commented Oct 19, 2022 •

edited

Loading

coveralls commented Oct 19, 2022 •

edited

Loading

mscuthbert Oct 26, 2022

MarkGotham Nov 1, 2022

MarkGotham commented Nov 1, 2022

mscuthbert Nov 2, 2022

MarkGotham Nov 2, 2022

mscuthbert Nov 2, 2022

mscuthbert commented Jan 30, 2023

MarkGotham commented Jan 30, 2023

mscuthbert left a comment

__eq__ comparison on time signatures #1457

__eq__ comparison on time signatures #1457

Conversation

MarkGotham commented Oct 10, 2022

MarkGotham commented Oct 10, 2022

mscuthbert left a comment

Choose a reason for hiding this comment

mscuthbert commented Oct 10, 2022

MarkGotham commented Oct 19, 2022 • edited Loading

coveralls commented Oct 19, 2022 • edited Loading

mscuthbert Oct 26, 2022

Choose a reason for hiding this comment

MarkGotham Nov 1, 2022

Choose a reason for hiding this comment

MarkGotham commented Nov 1, 2022

mscuthbert Nov 2, 2022

Choose a reason for hiding this comment

MarkGotham Nov 2, 2022

Choose a reason for hiding this comment

mscuthbert Nov 2, 2022

Choose a reason for hiding this comment

mscuthbert commented Jan 30, 2023

MarkGotham commented Jan 30, 2023

mscuthbert left a comment

Choose a reason for hiding this comment

`eq` comparison on time signatures #1457

`eq` comparison on time signatures #1457

MarkGotham commented Oct 19, 2022 •

edited

Loading

coveralls commented Oct 19, 2022 •

edited

Loading