Define key signatures without overwriting the --key command-line option

[nicolai-rostov]

Hello @syncopika,

I've formatted my edits as per your comments, and made the code work even if self.opts is undefined. Also, in addition to --title, and --names, I've added a --master command-line option to override LMMS master pitch.

Best, n.

[syncopika]

great idea for adding these options as flags @nicolai-rostov, thanks very much!

and the minor param will definitely be helpful in determining when I need to adjust some notes for the minor scales :)

[nicolai-rostov]

[...] do you think it might be possible for someone to want to specify a natural minor instead of a melodic minor (such that they wouldn't care about or want the raised 6th and/or 7th)? , I was replying to this question on the open issues page, but I think it's better for us to discuss this in private. So I will continue below, here, what I was saying there in reply to that question.

the minor param will definitely be helpful in determining when I need to adjust some notes for the minor scales :)

To tell the truth, when we get the code right, I think we'll be able to drop this minor boolean altogether. Ideally your code shouldn't be aware of music theory beyond what notes are diatonic in a given key signature.

I'm learning Python to be able to read your code and understand what is going on. I still don't get why commit 936fc94 wrongly interprets piano white key #59 for the key signature of G♭ as B♮, since #59 belongs to G♭ major. There is a bug somewhere. It won't do to fix singular notes on a scale per scale basis. Whatever is able to fix G♭ major has to fix all scales, major or minor, natural, melodic, harmonic, diatonic, chromatic, everything.

[syncopika]

I still don't get why commit 936fc94 wrongly interprets piano white key #59 for the key signature of G♭ as B♮, since #59 belongs to G♭ major.

In that commit I didn't yet add the enharmonic mapping for B natural so my program didn't know about changing B natural to Cb (see https://github.com/syncopika/mmp-to-MusicXML/commit/936fc94d42d14b3149c1b528b09289d1abb93f49#diff-d5930b5c7e6385856d6f20356338e9047c37f78044a4549911659ead2801be91R13).

I really appreciate you taking the time to learn Python just to understand my code :O! That means a lot.

[syncopika]

To tell the truth, when we get the code right, I think we'll be able to drop this minor boolean altogether. Ideally your code shouldn't be aware of music theory beyond what notes are diatonic in a given key signature.

Right, that would be ideal :).

However, at least the way I'm currently thinking about it, to be able to satisfy the requirements for https://github.com/syncopika/mmp-to-MusicXML/issues/7#issuecomment-2213948108, I think I need to check (within the add_note method) for 2 conditions in order to determine if I need to raise the 6 and/or 7 degrees:

• is the current note I'm going to add higher than the last note? (ascending check)
• am I currently in a minor key? (which is where the minor key flag comes in)

[nicolai-rostov]

I think we've solved this one now right? You're referencing an older commit where I didn't yet add the enharmonic mapping for B natural so my program didn't know about changing B natural to Cb (see 936fc94#diff-d5930b5c7e6385856d6f20356338e9047c37f78044a4549911659ead2801be91R13).

Yeah, but that was an ad hoc solution. It works for a couple of cases only. We need to come up with something more robust, something that will work for any note, in any key signature, in any scale whatsoever. We need to abstract from music theory.

However, at least the way I'm currently thinking about it, to be able to satisfy the requirements for #7 (comment), I think I need to check (within the add_note method) for 2 conditions in order to determine if I need to raise the 6 and/or 7 degrees:

Again, that might work for the 6th and 7th degrees in minor mode; but, again, that will be another _ad _hoc__ solution. The self.minor boolean was a bad idea. Sooner or later a user will open an issue about wrong notes.

I've got an idea. We need to get rid of music theory as much as possible. This is what we have to do instead:

1. We need, first of all, to mathematically calculate --- on the fly --- a list of what piano-key integer numbers correspond to a major scale built from the root of a user-supplied key;
2. Each integer in that list will map to 2 attributes { 'diatonic': boolean, 'degree': integer }
3. We set diatonic to true or false depending on how many steps it is above the latest root key found;
4. We step degree by 1 every time a new diatonic piano key is found;
5. If degree gets > 7, then we reset it to 1, and set a root variable to the current piano key integer.

Once we have the full list of piano keys with the attributes set, we go over the piano keys from the MMP file.

1. If the piano key is diatonic according to our list, then we look up the key signature to get the corresponding note, flat, and sharp.
2. If it is chromatic, it gets an <alter>1</alter>.

I can try to write all this in Python, if you allow me more freedom with space formatting, at least until we get it all working. :)

[nicolai-rostov]

In that commit I didn't yet add the enharmonic mapping for B natural so my program didn't know about changing B natural to Cb (see 936fc94#diff-d5930b5c7e6385856d6f20356338e9047c37f78044a4549911659ead2801be91R13).

But that's whole point. We shouldn't need to look up enharmonic mappings at all. Your code needs to get note names (C ... G) right from the start of the conversion process, with no adjustments later on. Any adjustment will flag a bug somewhere.

[syncopika]

Yeah, but that was an ad hoc solution. It works for a couple of cases only. We need to come up with something more robust, something that will work for any note, in any key signature, in any scale whatsoever. We need to abstract from music theory.

Ahh ok, I didn't realize there were cases where it wasn't working out 😟. Sorry about that! You're totally right, there needs to be a more robust strategy.

Your strategy sounds pretty good - definitely feel free to try it out (no worries about formatting 😄) and make a PR for it! I might have a go at trying to implement it as well :).

Thanks very much for the discussion on this feature - I'm learning quite a bit.

[nicolai-rostov]

Dear @syncopika,

I've just force-pushed the code under review to my command-line_options branch. Please have a look at it when you have the time. I need it to get accepted and merged onto your add-specified-key-signature-support branch, so that we have a common basis to work from, before I move on to the key signatures issue.

Now, I ran pytest and got: 8 failed, 10 passed. However, those 8 failed because the test script seems unable to find the generated XML files on a Linux system. I guess this has to do with using Windows and Unix directory separators: \ and /, in a single path at the same time.

Anyway, I converted the test files manually, and all generated XML files have the same shasum as those in the expected_output directory. So I can say my branch is able to pass the test, provided the test finds those generated files on my system.

generated

44db3107237e43069c9655438858b202f4473246  test_key_sig/a-chromatic.xml
dc68f30fd980498d85f79007a3b38f4d5db4399f  test_key_sig/a.xml
1c5c1f5fbcae8bf2f2e5ebc7e24277eee607a83c  test_key_sig/bb-chromatic.xml
eb111bded80e37aa55dc2c1ae04e45e7608a30bf  test_key_sig/bb.xml
4edec00a9cf7004663eef046e4a6b85a7c05ad58  test_key_sig/b-chromatic.xml
2b08f62ef6d56eeee60a31dca815d6394456202a  test_key_sig/b.xml
877d612858dae104b3ccd6c98654ea5b80506aab  test_key_sig/cb.xml
84e0324912a7e8ae8fcb6d5e9f4b8ccd0431706b  test_key_sig/db.xml
f31e7174a4a62369a87823f7316554d52cf1d704  test_key_sig/d-chromatic.xml
b5183b6c0a55c8d54f231211422a631994c5cfc0  test_key_sig/d.xml
38b3a5cc2a23dec23835f6778ef972cd02a2a2ad  test_key_sig/eb-chromatic.xml
4f97817c0773f8fe295aa61a1e82d8b4a84b16b2  test_key_sig/eb.xml
27033ed2eaa5e14182efa94f98aec71b5194d44d  test_key_sig/e-chromatic.xml
1e7b618ca7d062adb18408633c85d5522176e4a6  test_key_sig/e.xml
332d20c973601d4c927451b093432da94093e08e  test_key_sig/f-chromatic.xml
0a15a8fd127de4116d371cd1b2734f9dad42b30c  test_key_sig/f.xml
a0e4b228926e776dca35fcd24bf33c3497f5c3d7  test_key_sig/gb-chromatic.xml
91049b290ce0cb6a7d83cb4adf38c5bfb6b00174  test_key_sig/gb.xml
842f1da8038823acc1e0ec473baa3d776c866d1c  test_key_sig/g.xml

expected_output

44db3107237e43069c9655438858b202f4473246  test_key_sig/expected_output/a-chromatic.xml
dc68f30fd980498d85f79007a3b38f4d5db4399f  test_key_sig/expected_output/a.xml
1c5c1f5fbcae8bf2f2e5ebc7e24277eee607a83c  test_key_sig/expected_output/bb-chromatic.xml
eb111bded80e37aa55dc2c1ae04e45e7608a30bf  test_key_sig/expected_output/bb.xml
4edec00a9cf7004663eef046e4a6b85a7c05ad58  test_key_sig/expected_output/b-chromatic.xml
2b08f62ef6d56eeee60a31dca815d6394456202a  test_key_sig/expected_output/b.xml
877d612858dae104b3ccd6c98654ea5b80506aab  test_key_sig/expected_output/cb.xml
d4d3e8d496217233f2ffeb93739d8c2b9e6c922c  test_key_sig/expected_output/db-chromatic.xml
84e0324912a7e8ae8fcb6d5e9f4b8ccd0431706b  test_key_sig/expected_output/db.xml
f31e7174a4a62369a87823f7316554d52cf1d704  test_key_sig/expected_output/d-chromatic.xml
b5183b6c0a55c8d54f231211422a631994c5cfc0  test_key_sig/expected_output/d.xml
38b3a5cc2a23dec23835f6778ef972cd02a2a2ad  test_key_sig/expected_output/eb-chromatic.xml
4f97817c0773f8fe295aa61a1e82d8b4a84b16b2  test_key_sig/expected_output/eb.xml
27033ed2eaa5e14182efa94f98aec71b5194d44d  test_key_sig/expected_output/e-chromatic.xml
1e7b618ca7d062adb18408633c85d5522176e4a6  test_key_sig/expected_output/e.xml
332d20c973601d4c927451b093432da94093e08e  test_key_sig/expected_output/f-chromatic.xml
0a15a8fd127de4116d371cd1b2734f9dad42b30c  test_key_sig/expected_output/f.xml
a0e4b228926e776dca35fcd24bf33c3497f5c3d7  test_key_sig/expected_output/gb-chromatic.xml
91049b290ce0cb6a7d83cb4adf38c5bfb6b00174  test_key_sig/expected_output/gb.xml
842f1da8038823acc1e0ec473baa3d776c866d1c  test_key_sig/expected_output/g.xml

As you can see, they are byte-by-byte the same.

Thanks

[syncopika]

thanks @nicolai-rostov and apologies about the filepaths and the extra work you had to do! this looks good 👍 💯