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

Figured bass import and export for MusicXML and MEI #1613

Open
wants to merge 75 commits into
base: master
Choose a base branch
from

Conversation

mxordn
Copy link
Contributor

@mxordn mxordn commented Jun 18, 2023

I open the PR as a reopening of #1539 now based on the most recent version of music21.

It adds objects and the functionality to import figured bass information from MusicXML and MEI files and to export figuredbass.information to MusicXML.

Short description

(1) The firguredbass information is stored in a new object called FiguredBassIndication that inherits from a harmony.Harmony object. Within it uses the existing figuredBass.notation.Notation object, that was at some points exerted to fit the needs of an imported figured-bass tag.

A difference to the old version in March is now, that the object stores the original part (staff) where the information is located in the xml data of the score.

(2) The FiguredBassIndications objects are now inserted into the Measure objects of the corresponding part. My first attempt to store figured bass at the top level of stream object polluted the stream a bit and made the figures more disconnected from where they originally occur. The only link was actually the offset within the piece. Bringing together the info of pitch and figures afterwards while working with the music21 stream seemed a bit too much effort. The current position within a part and measure object makes it still accessible and also delivers a bit of context nearby.

The implementation is very much inspired by the way MEI v4 deals with figuredbass information. Except that in an MEI file parts are usually stored within a measure tag.

(3) I added some short excerpts of pieces with figured bass notation in MEI and MusicXML format to the LilypondTestSuite and to the tests in the MEI module. If this is a problem I can remove them.

TODOs

(1) Now, philological specialties of a figure may not be preserved during import an export of a score. This is maybe something that needs to addressed separately. It involves a decision whether a philological precision or a translation into something „computable“ is deserved or both. The notation of figures is rather complex and has a lot to do with the context in a score. For the moment I am happy with the current state.

Moritz Heffter and others added 30 commits February 26, 2023 15:57
fixed import and moved figuredBassIndication to harmony
Add utility for removing duplicated objects (e.g. keys, clefs) (cuthbertLab#1454)
@coveralls
Copy link

Coverage Status

coverage: 92.919% (-0.07%) from 92.985% when pulling a4b523e on mxordn:master into cf0719a on cuthbertLab:master.

@mscuthbert
Copy link
Member

Note that the test coverage dropped significantly with this PR suggesting that many features are not tested anywhere. Look at the coverage/coveralls report and see what parts of the system are not being tested and be sure that those are covered either by a unit test or a doctest or both.

@mscuthbert
Copy link
Member

besides things caught by lint/mypy/etc.:

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.

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

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.

only camelCase not underscore_case for exposed properties, etc.

limit abbreviations except inside functions. So .fib is out (fibonacci? fiberous bow?)

look at existing indentation in the system -- especailly for additions to figuredBass/notation

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

in FiguedBassIndication:

  • rename to FiguredBass
  • figs -- neds typing of list of what? remove None option and make default be () or ''
  • watch indentation there. See format elsewhere -- all arguments on one line or each argument on its own line,.
  • Python's not TypeScript: isFigure, part, _figs should be defined on the instance not the Class.
  • do not use str for "part" -- people will expect that "part" refers to a stream.Part object
  • 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

mei base:

harmFromElement -- type all attributes and tuple of what?
measureFromElement -- harmElements -- need precise typing.

musicxml

ScoreExporter -- need better typing, no undescore case. Why is fb_part here an int but in Figure it's a string?

getFiguredBassIndications -- are FiguredBassIndications to be at the Score level? Please explain. What about scores with multiple continuo instruments?

self.offsetFiguresInMeasure, tempFigureDuration: offsets can be OffsetQL -- float or Fraction. Don't call something "Duration" if it's not a Duration object; call it quarterLength or offset as appropriate.

Please explain why FiguredBassIndications need a separate iteration step from all other elements -- this is a strong -1 for me.

insertFiguredBassIndications: raise exception early and then don't need an indent. But note: an empty Stream is Falsy, so you'd be raising exceptions for empty voices, etc. But why is the stream being manipulated at this point? That's quite costly in terms of time -- all caches are being invalidated.


this is a very good start, but it looks like it has a lot of testing and refining before it can be approved and it's not entirely likely before July 1. Why not split into three PRs (and squash commits before staring a PR): one for the improvements to the object; one for MEI import; one for XML in/out? That way at least some of this is likely to be approved before the sabbatical.

@mxordn
Copy link
Contributor Author

mxordn commented Jun 19, 2023

Thanks for reviewing so quickly.

Your suggestion to split things up seems a good idea to me. To minimize the number of commits (I was doing that in very small steps), I would start from the current version and open up a clean history of changes. Is it something like that, what you intended?

I'm starting today with the FiguredBass object. Let's see how far we can get till July. I'll try my very best.

@mscuthbert
Copy link
Member

FiguredBass object was merged, feel free to take on the next part. :-)

1 similar comment
@mscuthbert
Copy link
Member

FiguredBass object was merged, feel free to take on the next part. :-)

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