Feature/figuredbass object #1614

mxordn · 2023-06-19T21:16:26Z

I'm opening this as a smaller step towards representing and importing figures bass information. PR #1613 is the big PR that is split here into three parts as suggested there.

One of the first things is to add the FiguredBass Object, that stores the information in the score/stream.

I'm comenting on the remarks by mscuthbert:

Leave the repr for fb.notation.Figure alone for the common case where hasExtension is False. Add "+extension" or "--" or something for that case. in general, touch as little as needed.

Done so now.

Make isExtension a property that computes automatically or at least is updated if number or figure is changed.

The Figure init does this now.

When adding an additional less-used feature to an existing method, generally make it keyword only. (such as extenders)

need typing on extenders -- no idea what it is expecting. (all new additions to m21 must be typed). Try to avoid X | None types where falsey X (like an empty tuple) can suffice.

I changes the handling of extenders now slightly and decided that they only can be parsed as underscores. Extenders are a rather big problem. For now also the musicxml and mei parser can work with the underscore char.

only camelCase not underscore_case for exposed properties, etc.

changed the snake case things

what is "originalString" doing in Modifier that is different than modifierString?

it stores an "original" version with the original values. E. g. a figure number 3 would not be added to a pure '#'

in FiguedBassIndication:

rename to FiguredBass

Done

figs -- neds typing of list of what? remove None option and make default be () or ''

Done

watch indentation there. See format elsewhere -- all arguments on one line or each argument on its own line,.

Alright didn't know that. Done.

Python's not TypeScript: isFigure, part, _figs should be defined on the instance not the Class.

Done

do not use str for "part" -- people will expect that "part" refers to a stream.Part object

renamed to "correspondingPart"

fig_notation --> notation, since it returns a Notation object (and no snake case). needs typing and docs. Currently violates these property: f = harmony.FiguredBassIndication(...); f.fig_notation = f.fig_notation should not change f.fig_notation

Should be solved. Although I'm not sure, if I got your point.

Hope this is going better in this way.

…Figures objects

coveralls · 2023-06-19T21:33:31Z

coverage: 93.037% (+0.01%) from 93.027% when pulling 88b8ed9 on mxordn:feature/figuredbass-object into 9951a8e on cuthbertLab:master.

mscuthbert

On the right track -- some changes requested (and more tests and documentation).

In general, objects do not keep track of what Part they are in; so I'd need a very compelling reason why FiguredBass needs a correspondingPart when ChordSymbols etc. don't.

Thanks!

mscuthbert · 2023-06-19T21:23:00Z

music21/figuredBass/notation.py

+prefixes = ['+', '#', '++', '##']
+suffixes = ['\\']
+
+modifiersDictXmlToM21 = {'sharp': '#',


indent as

modifiersDictXmlToM21 = { 'sharp' :'#', 'flat': 'b', ...etc. }

so that all keys begin on the same column and have four spaces showing they're inside a top-level dictionary. (same below)

mscuthbert · 2023-06-19T21:23:25Z

music21/figuredBass/notation.py

@@ -189,12 +234,15 @@ def __init__(self, notationColumn=None):
        self.origModStrings = None
        self.numbers = None
        self.modifierStrings = None
+        self.extenders:list[bool] = []


space before list[bool]

mscuthbert · 2023-06-19T21:23:43Z

music21/figuredBass/notation.py

        self._parseNotationColumn()
        self._translateToLonghand()

        # Convert to convenient notation
        self.modifiers = None
        self.figures = None
+        self.figuresFromNotationColumn: list(Figure) = []


list[Figure] -- square brackets not parentheses.

mscuthbert · 2023-06-19T21:24:35Z

music21/figuredBass/notation.py


        numbers = tuple(numbers)
        modifierStrings = tuple(modifierStrings)

+        # extenders come from the optional argument when instantionting the object.


typo. instantiating.

mscuthbert · 2023-06-19T21:24:48Z

music21/figuredBass/notation.py

+        # extenders come from the optional argument when instantionting the object.
+        # If nothing is provided, no extenders will be set.
+        # Otherwise we have to look if amount of extenders and figure numbers match
+        # 


remove blank #

mscuthbert · 2023-06-19T21:43:40Z

music21/harmony.py

+                    self._figs = ','.join(figureString)
+        else:
+            self._figs = ''
+        self._figNotation = notation.Notation(self._figs)


needs typing.

mscuthbert · 2023-06-19T21:46:38Z

music21/harmony.py

+
+
+    def __init__(self,
+                 figureString: str | list[str] | None = None,


After music21 moved to the world of typing extensions (around v6) figureString should just take a string. If someone wants to pass in a list of figures that can be a different keyword-only argument like figureStrings.

There are lots of cases in the current code that haven't moved to this paradigm because they're grandfathered in, but we're trying to make it so that new code mostly takes one type of object.

I would then add the two keywords figureString and figureStrings. If nothing at all is handed over, it will result in an empty FiguredBass.

mscuthbert · 2023-06-19T21:48:09Z

music21/harmony.py

+    def figNotation(self) -> notation.Notation:
+        return self._figNotation
+
+    @figNotation.setter
+    def figNotation(self, figs: str | list[str] | None):
+        self._figNotation = notation.Notation(figs)


as noted above, this doesn't pass reflexive setters. (also no abbreviations. This should just be notation)

Needs docs and tests.

it seems like there should be two sets of properties. One for getting/setting the Notation object, and one for getting/setting the string of figures -- changing one should change the other.

Good idea. I'm pushing a proposal.

mscuthbert · 2023-06-19T21:50:52Z

music21/harmony.py

+    It is created as a representation for <fb> tags in MEI and <figured-bass> tags in MusicXML.
+    The FiguredBassIndication object derives from the Harmony object and can be used
+    in the following way:
+    >>> fb = harmony.FiguredBass('#,6#', correspondingPart='P1')


space before.

mscuthbert · 2023-06-19T21:51:48Z

music21/harmony.py

+    def __repr__(self):
+        return f'<{self.__class__.__name__} figures: {self.figNotation.notationColumn} correspondingPart: {self.corresPart}>'


Music21Objects don't define __repr__ just _reprInternal, which would be just the part from figures: {...} to the end before > -- let the Music21Object __repr__ take care of the class name, etc.

Generally the representation just provides the most relevant information, so I think it could just be return self.notation.notationColumn. in .show('text') it'll be obvious what Part the FiguredBass object belongs to.

mscuthbert · 2023-06-20T17:19:02Z

Keeping track of the associated bass Note or Pitch object for the figure and the Key object seems like it could be important--since unlike RomanNumerals or ChordSymbols, a FB doesn't make sense without a bass (and unlike ChordSymbols but like RN it doesn't make sense without a Key object).

One thing that there are not yet tests on is .pitches -- we need to be able to get reasonable pitches out of the FB object. That's where breaking this PR up into its component parts has helped with the reasoning.

There may be parts of the RN code that might be factored out to make more reusable there. Do you mind if I push code to this PR? I think I have an idea of how things may work...

mxordn · 2023-06-20T19:40:47Z

Sure you can contribute to the PR. It’d be a pleasure. In general, it would be nice to have a pitch associated with the fb notation. Although I was careful with that, because there might be e.g. a rest with a figure in a score. In these cases the figure might just indicate the sonority, that sounds at a specific point (with the related bass note somewhere else in the score). I’d like to be able to preserve the possibility to either to have FiguredBass without a bass note explicitly specified or to be able to compute the related note out of a score. Therefore an offset is might be sufficient.

…

Am 20.06.2023 um 19:19 schrieb Michael Scott Asato Cuthbert ***@***.***>: Keeping track of the associated bass Note or Pitch object for the figure and the Key object seems like it could be important--since unlike RomanNumerals or ChordSymbols, a FB doesn't make sense without a bass (and unlike ChordSymbols but like RN it doesn't make sense without a Key object). One thing that there are not yet tests on is .pitches -- we need to be able to get reasonable pitches out of the FB object. That's where breaking this PR up into its component parts has helped with the reasoning. There may be parts of the RN code that might be factored out to make more reusable there. Do you mind if I push code to this PR? I think I have an idea of how things may work... — Reply to this email directly, view it on GitHub <#1614 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADOJL7IE7XOPAJ2UH44YXDXMHLRBANCNFSM6AAAAAAZMNEIRU>. You are receiving this because you authored the thread.

mscuthbert · 2023-06-21T01:28:30Z

yes -- there are definitely cases where the pitch being attached to is other than the one sounding (definitely in the case of 4-3 suspensions, etc.) so that's where a reference to a previous pitch would be helpful.

mscuthbert · 2023-07-01T08:55:17Z

I'm a little bit behind w/ other work, so I'll extend work on this into my sabbatical for a little bit (until July 10) -- if we can get this merged by then, I'll continue working on it.

mscuthbert · 2023-07-01T23:12:14Z

music21/figuredBass/notation.py

+                    extender = self.extenders[i]
+            else:
+                extender = False
+            figure = Figure(number, modifierString, extender)
            figure = Figure(number, modifierString)


this is immediately wiped out -- mistake? Not tested enough.

Check for what is not being covered by tests when adding new features please.

Moritz Heffter added 2 commits June 19, 2023 22:48

introduces FiguredBass object and makes some changes to Notation and …

b7ae781

…Figures objects

indentation update

94f76b4

mscuthbert requested changes Jun 19, 2023

View reviewed changes

corrections to notation PR cuthbertLab#1614

d6ee784

added requested changes to figuredbass and notation object

7b099f2

mscuthbert reviewed Jul 1, 2023

View reviewed changes

mscuthbert added 6 commits July 1, 2023 13:54

first commit for fixing issues in place.

c4609bf

move FiguredBass object to figuredBass.FiguredBass

c847113

Merge branch 'master' into pr/1614

2672974

lint and mypy

70f9f2c

Cover uncovered test lines. https://coveralls.io/builds/60910555

f232f9d

Check for what is not being covered by tests when adding new features please.

reuse your setters, Myke!

88b8ed9

mscuthbert approved these changes Jul 2, 2023

View reviewed changes

mscuthbert merged commit 24dcb41 into cuthbertLab:master Jul 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/figuredbass object #1614

Feature/figuredbass object #1614

mxordn commented Jun 19, 2023

coveralls commented Jun 19, 2023 •

edited

Loading

mscuthbert left a comment

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mxordn Jun 20, 2023

mscuthbert Jun 19, 2023

mxordn Jun 21, 2023

mscuthbert Jun 19, 2023

mscuthbert Jun 19, 2023

mscuthbert commented Jun 20, 2023

mxordn commented Jun 20, 2023 via email

mscuthbert commented Jun 21, 2023

mscuthbert commented Jul 1, 2023

mscuthbert Jul 1, 2023



		def __init__(self,
		figureString: str \| list[str] \| None = None,

		def __repr__(self):
		return f'<{self.__class__.__name__} figures: {self.figNotation.notationColumn} correspondingPart: {self.corresPart}>'

Feature/figuredbass object #1614

Feature/figuredbass object #1614

Conversation

mxordn commented Jun 19, 2023

coveralls commented Jun 19, 2023 • edited Loading

mscuthbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscuthbert commented Jun 20, 2023

mxordn commented Jun 20, 2023 via email

mscuthbert commented Jun 21, 2023

mscuthbert commented Jul 1, 2023

Choose a reason for hiding this comment

coveralls commented Jun 19, 2023 •

edited

Loading