-
Notifications
You must be signed in to change notification settings - Fork 403
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
base: master
Are you sure you want to change the base?
Conversation
fixed import and moved figuredBassIndication to harmony
…ht have to be added in the future
Mei import fb support
Add utility for removing duplicated objects (e.g. keys, clefs) (cuthbertLab#1454)
…was erroneously set to 0
Musicxml fb import
Co-authored-by: Jacob Walls <[email protected]>
Test figured bass merged into master
…into test-figured-bass
Test figured bass
version with Figures at top level
Test figured bass
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. |
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:
mei base: harmFromElement -- type all attributes and tuple of what? 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. |
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. |
FiguredBass object was merged, feel free to take on the next part. :-) |
1 similar comment
FiguredBass object was merged, feel free to take on the next part. :-) |
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.