MusicXML Parser for Magenta #214

jsawruk · 2016-09-15T19:55:56Z

Lightweight MusicXML parser for Magenta with no dependencies. Handles uncompressed (.xml) and compressed (.mxl) files. Translates MusicXML file into NoteSequences similar to how MIDI is imported.

Because MusicXML differs from MIDI, different information is available to Magenta. For example, MusicXML does not contain MIDI CC data. However, MusicXML does contain other information (such as dynamic markings) which may be more useful in certain scenarios, such as using Magenta to generate sheet music (MusicXML files) in addition to MIDI files.

This parser currently only supports input and does not support output to MusicXML files.

Four public domain MusicXML files are included for unit testing.

googlebot · 2016-09-15T19:55:58Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

adarob · 2016-09-15T20:56:37Z

@cghawthorne or @iansimon: can one of your take this review?

cghawthorne · 2016-09-15T21:02:26Z

Yup, I can definitely look at it. Wouldn't mind a second pair of eyes, though.

@jsawruk we'll start the review once the CLA is signed. Thanks again for your work on this!

iansimon · 2016-09-15T21:06:39Z

I'll take a look also.

-Ian

On Thu, Sep 15, 2016 at 2:02 PM cghawthorne notifications@github.com
wrote:

Yup, I can definitely look at it. Wouldn't mind a second pair of eyes,
though.

@jsawruk https://github.com/jsawruk we'll start the review once the CLA
is signed. Thanks again for your work on this!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#214 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABTHb6_coRvnUz7DefMLSn9WPol1tVzlks5qqbJwgaJpZM4J-R_e
.

jsawruk · 2016-09-23T17:45:59Z

I signed it!

On Thu, Sep 15, 2016 at 3:56 PM, googlebot notifications@github.com wrote:

Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project. Before we can look at your
pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/
https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll

verify. Thanks.

If you've already signed a CLA, it's possible we don't have your
GitHub username or you're using a different email address. Check your
existing CLA data https://cla.developers.google.com/clas and verify
that your email is set on your git commits
https://help.github.com/articles/setting-your-email-in-git/.

If you signed the CLA as a corporation, please let us know the
company's name.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#214 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAZxWQclQSKQz5a6tlNA4FwAtj84Zpu2ks5qqaLdgaJpZM4J-R_e
.

cghawthorne · 2016-09-23T18:24:11Z

@jsawruk the cla bot still doesn't seem happy. Can you try replying using the github web interface?

jsawruk · 2016-09-23T18:29:15Z

I updated the contact info email address on the CLA to match the email address on my Github account. Hopefully this resolves the issue.

cghawthorne · 2016-09-23T18:30:56Z

Might need to re-reply with "I signed it!"

jsawruk · 2016-09-27T16:36:24Z

I signed it!

willnorris · 2016-09-27T16:51:39Z

@jsawruk: sorry about the confusion here. It looks like we're not able to map your GitHub username to the email address used in your commits. Could you add your *@jwpepper.com email address to your GitHub account? That should take care of this.

jsawruk · 2016-09-27T17:00:23Z

Ok, I have added that email address to my Github account and it is now verified.

googlebot · 2016-09-27T17:00:25Z

CLAs look good, thanks!

iansimon

A number of tests are failing because a nonexistent field called "bpm" is being accessed in the Tempo object in the NoteSequence protobuf. This field should be called "qpm" and is defined as quarter notes per minute. Here's the message on the Magenta mailing list describing the change from bpm to qpm: https://groups.google.com/a/tensorflow.org/d/msg/magenta-discuss/b33XYjXjj-U/ozfaXAvwDAAJ

It looks as though MusicXML also defines tempo as quarter notes per minute, so you probably want to call the field "qpm" in your Tempo class in musicxml_parser.py as well.

jsawruk · 2016-09-27T22:00:51Z

In process of updating my code. I just rebased against master, and am now getting this error when running the unit tests:

Traceback (most recent call last):
  File "musicxml_parser_test.py", line 20, in <module>
    from musicxml_reader import *
  File "/Users/jsawruk/magenta/magenta/music/musicxml_reader.py", line 29, in <module>
    from magenta.protobuf import music_pb2
ImportError: No module named magenta.protobuf

What changed?

cghawthorne · 2016-09-28T00:54:38Z

I think you're running into a merge conflict with #223. We recently moved a bunch of our modules that used to be in /lib to /music. You'll probably also want your musicxml code to be in /music now.

jsawruk · 2016-09-28T14:26:05Z

I'm still getting errors. I checked out the master branch at commit 0056294, which includes #223. When I try to import using python, I get this error:

>>> from magenta.protobuf import music_pb2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "magenta/__init__.py", line 26, in <module>
    import magenta.music.melodies_lib
ImportError: No module named music.melodies_lib

This does include the new /music directory, and the BUILD file there looks correct.

iansimon · 2016-09-28T21:56:37Z

Are you having issues when running bazel test, or only in interactive python? When working on libraries for core magenta, we recommend testing via bazel.

jsawruk · 2016-09-28T22:23:55Z

I wasn't running the bazel test correctly, so I tried it in interactive Python. Once I ran the bazel test correctly, I was able to resolve the issue; the bazel tests now pass on my system.

cghawthorne

Thanks for your work here! I've mostly looked at style/structure issues thus far and will get to more specifics in a later pass.

cghawthorne · 2016-09-29T22:31:25Z

magenta/music/BUILD

@@ -171,7 +171,7 @@ py_library(
 py_test(
    name = "midi_io_test",
    srcs = ["midi_io_test.py"],
-    data = ["//magenta/testdata"],
+    data = ["//magenta/testdata:testdata"],


any reason for this change? the :testdata target is implicit for the /testdata package

No idea. Maybe I was having issues initially, but the tests pass now, so it should not have :testdata. Thanks.

cghawthorne · 2016-09-29T22:31:45Z

magenta/music/BUILD

@@ -180,6 +180,26 @@ py_test(
 )

 py_library(
+    name = "musicxml_reader",
+    srcs = ["musicxml_reader.py", "musicxml_parser.py"],


we prefer for each python file to have its own py_library target

Fixed. Will be in next commit.

cghawthorne · 2016-09-29T23:29:23Z

magenta/music/musicxml_parser.py

+# Constants
+
+# Number of MIDI ticks per beat. Required by NoteSequence structure
+TICKS_PER_BEAT = 960


We've standardized on magenta.music.constants.STANDARD_PPQ

Fixed in next commit.

cghawthorne · 2016-09-30T21:45:31Z

magenta/testdata/BUILD

@@ -34,5 +34,9 @@ filegroup(
        "example_is_drum.mid",
        "notesequences.tfrecord",
        "tfrecord_iterator_test.tfrecord",
+        'clarinet_scale.xml',


I think I'd recommend putting these in music/testdata because they'll only be used to verify your importer. Also, when you do that, please update the README in that folder to describe the files.

Good idea. This is fixed and will be in the next commit.

cghawthorne · 2016-09-30T21:49:31Z

magenta/music/musicxml_parser.py

+
+# Global variables
+# Default to one division per measure
+current_divisions = 1


package-level constants should be named in uppercase: https://google.github.io/styleguide/pyguide.html#Naming

Already fixed after using pylint.

cghawthorne · 2016-09-30T21:59:39Z

magenta/music/musicxml_parser.py

+current_tempo = 120
+
+# Duration of a single beat in seconds
+current_seconds_per_beat = 0.5


it looks like some of these package-level variables actually get modified in functions below. I would prefer that variables related to parsing a file live in the function or instance for parsing that file. For example, right now this class isn't threadsafe because every invocation modifies the same variables, but there's no reason it couldn't be threadsafe because this shared state isn't actually needed.

I don't have a good solution for this at the moment. I know the code isn't threadsafe because I am mutating global variables, but parsing MusicXML files can be tricky. MusicXML files (like most/all? xml files) do not guarantee order of elements, only that the elements exist. For example, let's say there is a tempo marking over beat 1 in the first measure of quarter = 96. Ideally, I'd like to know the tempo before parsing the note on beat one, but MusicXML does not guarantee this. That example is trivial, but it gets more complicated when keeping track of time and you encounter the <forward> and <backup> elements.

I'd like to keep track of this state somehow, but my only two potential solutions were to pass around a state object to every class, or to use globals. Since I wanted to get things working quickly, I resorted to globals. I can now refactor to something more elegant and threadsafe. Do you have any alternative solutions to this problem? If not, I'll need to pass around some object that maintains these state variables.

Yeah, I think you could use an instance variable rather than a global variable. So you could create an instance of the parser for every MusicXML file you're parsing and have instance variables to keep track of parsing state while you're going through the file.

cghawthorne · 2016-09-30T22:03:01Z

magenta/music/musicxml_parser.py

+    return score_str
+
+
+class Part:


All of these classes should have a docstring describing what information they contain.

cghawthorne · 2016-09-30T22:04:34Z

magenta/music/musicxml_parser.py

+    midi_pitch = (12 + pitch_class) + (int(octave) * 12)
+    return midi_pitch
+
+  def __str__(self):


Is this method outputting a particular format, or just something to make debugging easier to read? Would probably be good to document in the docstring for this method.

If you're asking about the str function, it was just to make my debugging easier as I was developing. Would you like me to remove the str method altogether, or do you have a proposed format that would be helpful to you?

This is fine, just wanted to make sure I understood the intention.

cghawthorne · 2016-09-30T22:05:26Z

magenta/music/musicxml_parser_test.py

+from musicxml_reader import *
+import tensorflow as tf
+
+# self.flute_scale_filename contains an F-major scale of 8 quarter notes each


This might fit better in the docstring for the class.

Moved to docstring. Will be in the next commit.

cghawthorne · 2016-09-30T22:06:25Z

magenta/music/musicxml_reader.py

+          note.pitch = musicxml_note.pitch[1] # Index 1 = MIDI pitch number
+          note.velocity = musicxml_note.velocity
+
+  # TODO(@douglaseck): Estimate note type (e.g. quarter note) and populate


Is this TODO still relevant?

Yes, I feel this is extremely relevant based on conversations I had with you and @douglaseck. With MusicXML, we can get the exact note length. With MIDI, we just get a ticks duration. @douglaseck was suggesting a quantization algorithm when reading MIDI data to feed into your model. Since MusicXML specifies the exact note type, I'd like to also supply that to your model. However, I didn't implement it because I wasn't sure what format you were expecting.

My guess is:

Whole note = 1

Quarter note = 1/4

Dotted Quarter = 3/8

Triplet Eighth = 1/12
etc.
Is this correct?

Yes, that looks correct to me. Feel free to start filling out those fields.

iansimon · 2016-09-30T22:34:44Z

I recommend running pylint on your code. You can even get something close to Google's linter configuration, described in this StackOverflow answer: http://stackoverflow.com/questions/29597618/is-there-a-tool-to-lint-python-based-on-the-google-style-guide

Basically, install pylint, download that googlecl-pylint.rc file, then run:

pylint --rcfile=/path/to/googlecl-pylint.rc <your files>

jsawruk · 2016-10-04T18:46:26Z

@iansimon: Thanks. I'm linting the code right now using that pylint configuration. Not done yet, but soon. Then I will address remaining issues raised by @cghawthorne.

jsawruk · 2016-10-05T18:54:46Z

@iansimon: I am getting a pylint error that I don't know how to resolve.

Bad option value 'g-import-not-at-top' (bad-option-value)

Please compare with the code here:
https://github.com/tensorflow/magenta/blob/master/magenta/music/midi_io.py#L21

iansimon · 2016-10-17T18:14:19Z

There seems to be a missing file el_capitan.xml being referred to in your tests. Did you forget to check this in?

jsawruk · 2016-10-17T18:22:31Z

@iansimon: Thanks, I missed that when I changed where the test files are located.

iansimon · 2016-10-19T21:46:43Z

magenta/music/musicxml_parser.py

+
+    xml_duration = xml_forward.find('duration')
+    forward_duration = int(xml_duration.text)
+    midi_ticks = forward_duration * (constants.STANDARD_PPQ / CURRENT_DIVISIONS)


You have CURRENT_DIVISIONS commented out above.

iansimon · 2016-10-19T22:18:50Z

I fixed many of the lint errors, mainly just ones that required no understanding of the code. There are still several that remain, largely missing/malformed docstring sections. There's also a CURRENT_DIVISIONS variable in musicxml_parser.py that is commented out in some places but not others.

jsawruk · 2016-10-19T22:20:43Z

@iansimon: Thanks for fixing the lint errors. I have been extremely busy at work and was hoping to work more on this code at the Classical Music Hack Day at MIT this coming weekend.

jsawruk · 2016-10-26T15:12:09Z

I believe this latest commit addresses all issues raised so far. I have implemented the note.numerator and note.denominator support (@douglaseck's TODO)

…Fix tempo parsing errors

… MusicXMLParserState object

…ables

… by reading the symbolic note durations from the MusicXML. This completes Douglas Eck's TODO.

…Magenta. Also raise exception for unknown pitch steps

… handle parse errors. Fix MIDI program and channel mapping

…ciated unit test

…a single compressed MXL file

cghawthorne · 2016-11-02T21:50:23Z

Hey Jeremy, I think this is looking pretty good! We might need some followup changes, but I think it's in a good shape to check in now!

cghawthorne self-assigned this Sep 15, 2016

cghawthorne assigned iansimon Sep 27, 2016

iansimon requested changes Sep 27, 2016

View reviewed changes

cghawthorne suggested changes Sep 30, 2016

View reviewed changes

iansimon reviewed Oct 19, 2016

View reviewed changes

Jeremy Sawruk added 25 commits November 2, 2016 15:40

Set default MIDI info for each part. Set default key signature to C. …

5fd89d6

…Fix tempo parsing errors

Remove debug code

e24e072

Rename BPM to QPM. Move files from magenta/lib/ to magenta/music/

fe7252c

Update musicxml_parser.py to conform with pylint

4355f10

Fix bazel tests for updated musicxml_parser

819ea21

Fix pylint errors in musicxml_reader.py and musicxml_parser_test.py

f348b62

Update changes requested by cghawthorne

05d8bf0

Begin refactoring of MusicXML parser to replace global variables with…

720753b

… MusicXMLParserState object

Add large MusicXML file

8e6473a

Update MusicXML parser to rebase with master, remove some global vari…

57996f3

…ables

Remove globals from musicxml_parser.py

81b673d

Add support for note.numerator and note.denominator in sequence notes…

b4c3eec

… by reading the symbolic note durations from the MusicXML. This completes Douglas Eck's TODO.

Update comments and naming conventions in parser

afd42eb

Prepend private methods in musicxml_parser.py with double underscores

222d0f2

Remove backslash continuation for long lines

855c0f6

Move duration typeratiomap to class level constant in musicxml_parser.py

14e55a1

Uncomment unit tests

71ecca7

Remove Part.transposes property from musicxml_parser

7040c4c

Remove Measure.transpose property from musicxml_parser

66a66e7

Add additional branches to parser to handle MusicXML tags ignored by …

e39588c

…Magenta. Also raise exception for unknown pitch steps

Remove pylint command from musicxml_reader.py

3c1c21d

Add try/except when parsing XML using ElementTree

3a585d7

Add unit tests for transposition. Add MusicXMLParseException class to…

5afa30c

… handle parse errors. Fix MIDI program and channel mapping

Add support for compressed MXL files with multiple rootfiles and asso…

fa38cb8

…ciated unit test

Raise MusicXMLParseException if multiple MusicXML files found within …

4989b4d

…a single compressed MXL file

cghawthorne approved these changes Nov 2, 2016

View reviewed changes

iansimon approved these changes Nov 2, 2016

View reviewed changes

cghawthorne merged commit be3ec41 into magenta:master Nov 2, 2016

jsawruk mentioned this pull request Nov 2, 2016

Fix Issue #341: Missing flute_scale_with_png.mxl file #342

Closed

MusicXML Parser for Magenta #214

MusicXML Parser for Magenta #214

Conversation

jsawruk commented Sep 15, 2016

googlebot commented Sep 15, 2016

adarob commented Sep 15, 2016

cghawthorne commented Sep 15, 2016

iansimon commented Sep 15, 2016

jsawruk commented Sep 23, 2016

verify. Thanks.

cghawthorne commented Sep 23, 2016

jsawruk commented Sep 23, 2016

cghawthorne commented Sep 23, 2016

jsawruk commented Sep 27, 2016

willnorris commented Sep 27, 2016

jsawruk commented Sep 27, 2016

googlebot commented Sep 27, 2016

iansimon left a comment

Choose a reason for hiding this comment

jsawruk commented Sep 27, 2016

cghawthorne commented Sep 28, 2016

jsawruk commented Sep 28, 2016

iansimon commented Sep 28, 2016

jsawruk commented Sep 28, 2016

cghawthorne 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

jsawruk Oct 5, 2016 • edited

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

iansimon commented Sep 30, 2016

jsawruk commented Oct 4, 2016

jsawruk commented Oct 5, 2016

iansimon commented Oct 17, 2016

jsawruk commented Oct 17, 2016

Choose a reason for hiding this comment

iansimon commented Oct 19, 2016

jsawruk commented Oct 19, 2016

jsawruk commented Oct 26, 2016

cghawthorne commented Nov 2, 2016

jsawruk Oct 5, 2016 •

edited