Python 3 and jazz updates #63
Conversation
Update import and raise syntax for Python 3.x (now supports mingus.core.*)
Changed conventions to Python 3 e.g. imports, line breaks, printing, xrange Fixed some type issues Added some jazz chord support (chord types, lower case as minor convention (moving away from mingus' existing diatonic/classical approach)) Added jazz scales: minor modes, altered scales, and some extras Updated midi support to work with binary strings (Python 3) and fixed some event handling (note-off)
|
Happy to discuss edits/conflicts, let me know. |
|
Hi @samg7b5! Thanks a lot for the contribution. An older proposal for Python 3 support has already been merged. Of course if you have any specific feedback do feel free to bring it. About the jazz updates, I'll be happy to review them, BUT that's nearly impossible in this PR in its current form. There's a lot of conflicts, and an overwhelmingly large set of changes due to this Python 3 rework. So I'd suggest that you isolate the functional changes that you wanted to introduce, removing the Python 3 updates that are already present in the Please ping me back if you need some help or advice at doing this. |
|
Hey @edudobay , I took a look at these conflicts and removed most of my edits to respect the existing Py3 edits that had already been implemented. There are just a few notable edits remaining from my side (plus the obvious additional jazz functionality in chords/scales): To This was because my MIDI files were being read in and having their octave multiplied by a non-integer factor, which effectively vertically stretched the pitches. I also added (around line 360)
This was to fix reading in MIDI created by Lilypond (Lilypad is a typo in the above) which in mingus was interpreted with notes that didn't end (i.e. kept playing) after their duration was passed. PS. I removed my jazz numeral functionality, so 'I7' should be interpreted as 'IMaj7' as before, and lowercase won't imply a minor chord. |
|
That's quite an impressive amount of work! In fact you have uncovered some bugs in MIDI-related code, and unfortunately that part is lacking automated tests. I'll shortly try to give specific feedback on your changes but I have to warn you that making a lot of unrelated changes in one single commit makes a PR much harder to review — and also can make maintenance harder in the future. The way I see this as a community-faced project, we should aim to make changes as transparent as possible and be responsible about those changes. The best way to handle this PR as I see it is to split it into smaller, semantic commits, with one type of change for each. My top priority is reviewing your fixes to broken behavior, so I'll start by the MIDI-related stuff and other changes to Note. |
| self.bar = [] | ||
| self.current_beat = 0.0 | ||
| return self.bar | ||
|
|
||
| def set_chord_notes(self, chords): | ||
| self.chords = chords |
There was a problem hiding this comment.
It is unclear to me why you added chords and temporal_notes to this class. It seems they are not used elsewhere.
There was a problem hiding this comment.
Not sure that I added this, maybe from a historic version of mingus? Guess it's fine to ignore
| @@ -272,7 +274,7 @@ def __int__(self): | |||
| res += 1 | |||
| elif n == "b": | |||
| res -= 1 | |||
| return res | |||
| return int(res) | |||
There was a problem hiding this comment.
It's not clear why this int should be needed. Did you ever get a non-int here? If so, maybe there's another bug to uncover.
There was a problem hiding this comment.
Sorry also not sure why I added this - I'm guessing it was from a type bug but I'm afraid I can't recall
|
|
||
| # added as was getting erros from play_Bar.set_key | ||
| if type(other) == str: | ||
| other = Note(other) |
There was a problem hiding this comment.
Seems to me we should not compare Notes and strings by implicit conversion, but instead raise a TypeError to uncover bugs.
| '7b5': ' dominant flat five', | ||
| 'hendrix': ' hendrix chord', | ||
| '7b12': ' hendrix chord', | ||
| '5': ' perfect fifth', |
There was a problem hiding this comment.
You should run make format prior to your commits to avoid accidentally committing undesired coding style changes.
| @@ -684,7 +726,7 @@ def I7(key): | |||
|
|
|||
|
|
|||
| def ii(key): | |||
| return supertonic(key) | |||
| return supertonic(key) # note that progressions.to_chords['ii'] will come and pick this up (major 2nd chord due to mingus' classical approach). Instead, that fn has been re-written to try to except lower-casing and add a minor suffix | |||
There was a problem hiding this comment.
Is this comment up-to-date? Seems this is about the changes you didn't keep.
There was a problem hiding this comment.
Correct, I think we should disregard this now as I removed the notion of jazz chord notation (originally I was using lowercase to imply minor but removed that functionality)
| n.channel = event["channel"] | ||
| n.velocity = event["param2"] | ||
| n = Note(notes.int_to_note(event['param1'] % 12), | ||
| np.floor(event['param1'] / 12)) # this was event['param1']/12 - 1 but that gives skewed, incorrect octaves |
There was a problem hiding this comment.
I think instead of floor we can simply use integer division: event["param1"] // 12.
Even if floor was needed, the one from NumPy returns a float instead of an int, and pulls in an unnecessary dependency (numpy is only required for the FFT module). The floor from the built-in math module would fit in perfectly.
There was a problem hiding this comment.
Already fixed on master (due to #64)
| fp.read(chunk_size / 2) | ||
| self.bytes_read += chunk_size / 2 | ||
| # fp.read(chunk_size // 2) | ||
| # self.bytes_read += chunk_size // 2 |
There was a problem hiding this comment.
Again, this seems like a debug thing. Indeed the divisions here should be integer divisions, but it seems to me there's no reason to remove this code and it should be uncommented.
There was a problem hiding this comment.
This might have been coming from LilyPond midi files - maybe worth keeping an eye on this in future
| @@ -332,6 +355,12 @@ def parse_midi_event(self, fp): | |||
| raise IOError("Couldn't read MIDI event parameters from file.") | |||
| param1 = self.bytes_to_int(param1) | |||
| param2 = self.bytes_to_int(param2) | |||
|
|
|||
| # "It is common for a note on with 0 velocity to be interpreted as NOTE OFF." https://stackoverflow.com/questions/48687756/midi-note-on-event-without-off-event | |||
| # this should resolve Lilypad midi files which don't seem to use ec 8 to signify NOTE OFF | |||
There was a problem hiding this comment.
Nice one. Did you mean LilyPond instead of Lilypad?
| # | ||
| # SG: | ||
| # Have needed to change strings to bstrings to get concat to work | ||
| # |
There was a problem hiding this comment.
This type of comment would be more suited to the commit message. But I think this one is outdated and just unneeded.
| @@ -256,7 +257,7 @@ def set_key(self, key="C"): | |||
|
|
|||
| def key_signature_event(self, key="C"): | |||
| """Return the bytes for a key signature event.""" | |||
| if key.islower(): | |||
| if str(key).islower(): | |||
There was a problem hiding this comment.
Nice. I think this solves it for now. In the future we should use stricter types.
|
Thanks @edudobay - just for context, it's not the best structured PR because when I was working on it I didn't really consider adding back into the project, but then afterwards figured I probably should share my updates back to the community. Hence the random comments and things, apologies for that. A couple of the edits that you've picked up on look like things that I never actually made myself - potentially diff against edits that have already been incorporated into the base in the meantime? These are the things like temporal_note, I can't recall adding (or needing to add) that in myself, as you rightly noted in your response. For simplicity, I'd suggest we only keep my MIDI fixes and the new jazz scales since these are quite clear. What do you think? I can take a look at the requested changes you've flagged above, but could you please advise on how I should go about splitting into different commits and re-submitting? Cheers! |
|
Hey @samg7b5, sorry I missed your comment for a few minutes. I was investigating the MIDI issues that were discussed in #64 and saw that further changes were needed. So I expedited a fix, including your changes and some other ones, that is already present in the master branch – mentioning your name in the commit message for credit :) About the other issues, I'd propose we add the jazz scales first, and then we can handle what's left (if needed). What do you think? |
|
@edudobay no problem at all! Thanks for the mention. Agree, let's add the jazz scales as it seems pretty much everything else is covered. And I'll try to do cleaner commits in future (I'm working on a higher-level jazz composition and analysis API that I'm calling Sidewinder, so may end up dipping into mingus again)! Please let me know if you need anything from me with regards to incorporating the jazz scales (they should all be quite clearly laid out in the scales.py file) and/or finalising or closing this PR? This is my first GitHub contribution so this is all pretty new to me :) |
|
|
||
| def generate(self, i): | ||
| print('Please use ascending() and descending() methods instead of generate() for SUHMM scales') | ||
| return False |
There was a problem hiding this comment.
It seems that returning False would totally break the expectations of the caller. I think that you should either raise an exception or accept to run the default generate normally. (In the latter case you could issue a warning, using warnings.warn())
|
@samg7b5 Sorry if I've been a bit harsh on some comments. Welcome and congrats on your first GitHub contribution :) Well, for the PR to be ready to merge we'll have to isolate the changes in |
|
Will open a new PR #66 with scales (and the couple of approved midi changes) isolated. |
Added various Python 3 support, small midi fixes, jazz features (chords, scales etc.). Please refer to commit notes for more details on each category.
Please note: some edits were made to move away from mingus' diatonic/classical numeral parsing and towards jazz notation (e.g. I7 was Imaj7, should now give Idom7). Would suggest reviewing and removing this code if not desirable for the package. See core/progressions.py and progressions.to_chords().
Apologies that this is not a well-structured pull request as I was originally editing the package for my own local usage.