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

Roman Numerals: Don't substitute d for 0 in add10 #1635

Merged

Conversation

malcolmsailor
Copy link
Contributor

add10 annotations (which occur when converting some DCML files) were causing a bug because the 0 was being converted to o, leading to parsing exceptions.

This PR updates the replace logic for 0 so as not to match this case, and adds a test case to the unit test.

@coveralls
Copy link

Coverage Status

coverage: 93.036%. remained the same when pulling 89e0b5a on malcolmsailor:preserve-10-in-figures into 8baa273 on cuthbertLab:master.

@jacobtylerwalls
Copy link
Member

Thanks for the fix and test!

@jacobtylerwalls jacobtylerwalls merged commit fdb126c into cuthbertLab:master Aug 27, 2023
6 checks passed
@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Aug 27, 2023

For future work with regexes, esp. in such a common code path like RN instantiation, it would be good to do timing tests when considering alternatives. I think without the lookbehind you can get an improvement:

>>> from timeit import timeit
>>> timeit("re.sub(r'(?<!\d)0', 'o', 'viio7')", setup='import re')
0.37872337497537956
>>> timeit("re.sub(r'(\\D)0', r'\1o', 'viio7')", setup='import re')
0.324621667037718
>>> timeit("re.sub(r'(?<!\d)0', 'o', 'vii07')", setup='import re')
0.43934187496779487
>>> timeit("re.sub(r'(\\D)0', r'\1o', 'vii07')", setup='import re')
0.3506033340236172

@malcolmsailor
Copy link
Contributor Author

If we're worried about speed perhaps it's worth doing a simple membership check to see if 0 is in the string. With a test where half of the figures contain 0 we get ~50% speedup; since 0 is presumably less common than that in a more representative sample of chord figures we'd probably get a much larger speedup.

In [8]: import re
   ...: from timeit import timeit
   ...: 
   ...: 
   ...: def f(fig):
   ...:     if "0" in fig:
   ...:         fig = re.sub(r"(\\D)0", r"\1o", fig)
   ...:     return fig
   ...: 

In [9]: def g(fig):
   ...:     return re.sub(r"(\\D)0", r"\1o", fig)

In [10]: timeit(
    ...:     "[f(fig) for fig in ['I', 'viio7', 'vii07', 'V[add10]']]",
    ...:     setup="from __main__ import f",
    ...: )
Out[10]: 1.833819167004549

In [11]: timeit(
    ...:     "[g(fig) for fig in ['I', 'viio7', 'vii07', 'V[add10]']]",
    ...:     setup="from __main__ import g",
    ...: )
Out[11]: 3.177722458000062

TimFelixBeyer added a commit to TimFelixBeyer/music21 that referenced this pull request Sep 2, 2023
commit f457a2b
Merge: fdb126c 8b76a3f
Author: Michael Scott Asato Cuthbert <[email protected]>
Date:   Wed Aug 30 22:54:36 2023 -1000

    Merge pull request cuthbertLab#1634 from malcolmsailor/add-2-digit-figures

    TSV converter bug fix: don't add 'd' prefix to 2-digit added tones like `[add13]`

commit fdb126c
Author: malcolmsailor <[email protected]>
Date:   Sun Aug 27 10:42:42 2023 -0600

    Roman Numerals: Don't substitute `d` for `0` in `add10` (cuthbertLab#1635)

commit 8b76a3f
Author: Malcolm Sailor <[email protected]>
Date:   Sat Aug 19 10:20:48 2023 -0400

    remove trailing ws

commit 9b5e387
Author: Malcolm Sailor <[email protected]>
Date:   Sat Aug 19 09:50:07 2023 -0400

    update

commit d3af7bf
Author: Malcolm Sailor <[email protected]>
Date:   Sat Aug 19 08:04:09 2023 -0400

    wip

commit 3babd94
Author: Malcolm Sailor <[email protected]>
Date:   Mon Jul 17 14:10:56 2023 -0600

    don't match 'add13' when performing dominant 'd' substitution

commit 8baa273
Merge: 2a28ab1 9f35bbb
Author: Michael Scott Asato Cuthbert <[email protected]>
Date:   Thu Jul 13 14:07:36 2023 -1000

    Merge pull request cuthbertLab#1629 from TimFelixBeyer/patch-5

commit 9f35bbb
Author: Tim Beyer <[email protected]>
Date:   Thu Jul 13 23:56:39 2023 +0200

    fix flake

commit 4c5ee5f
Author: Tim Beyer <[email protected]>
Date:   Tue Jul 11 23:46:22 2023 +0200

    Remove use of hasElement

commit 8141355
Author: TimFelix <[email protected]>
Date:   Sat Jul 8 20:28:08 2023 +0200

    Remove uses of `hasElement`

commit d43ec76
Author: Tim Beyer <[email protected]>
Date:   Sat Jul 8 20:16:18 2023 +0200

    Speed up __contains__ by 2x and deprecate `hasElement`
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