Implemented jazz minor scale and relative key functionality on harmonic, melodic, and jazz minors#1044
Implemented jazz minor scale and relative key functionality on harmonic, melodic, and jazz minors#1044Phantasmogenesis wants to merge 4 commits intocuthbertLab:masterfrom
Conversation
jacobtylerwalls
left a comment
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks, Jazz minor will be a nice addition as I see it. Some questions about the rest. Cheers, Jacob
music21/scale/__init__.py
Outdated
| self.relativeMinorDegree = 1 | ||
| self.relativeMajorDegree = 3 |
There was a problem hiding this comment.
Likewise, can you explain why this is needed?
There was a problem hiding this comment.
Without this code, an error message like this is given when using the getRelativeMinor/getRelativeMajor functions on the harmonic/melodic/jazz minor scales:
AttributeError: 'AbstractHarmonicMinorScale' object has no attribute 'relativeMinorDegree'
There was a problem hiding this comment.
Ah, got it, any of the non-natural minors, thanks. Yes, this should work because it's a method on DiatonicScale.
Can we move these attribute assignments to buildNetwork()? That's where they seem to be in all other cases.
jacobtylerwalls
changed the title
Added description to jazz minor scale, rearranged so harmonic minor and melodic minor would be directly adjacent, and removed certain unnecessary comments.
jacobtylerwalls
changed the title
mscuthbert
left a comment
mscuthbert
left a comment
There was a problem hiding this comment.
Making great progress. Happy to add this to music21!
| (The usage of this collection as a scale, is quite ahistorical for | ||
| (The usage of this collection as a scale is quite ahistorical for | ||
| Western European classical music, but it is common in other parts of the | ||
| world, but where the term "HarmonicMinor" would not be appropriate). |
There was a problem hiding this comment.
thanks for the space, but the "but" should remain
There was a problem hiding this comment.
Right, because "where" is not delimiting certain parts of the world where the term is/is not meaningful.
| <music21.pitch.Pitch B4> | ||
| >>> sc.pitchFromDegree(1) # scale degree 1 is treated as lowest | ||
| <music21.pitch.Pitch E4> | ||
| >>> sc = scale.JazzMinorScale() |
There was a problem hiding this comment.
space above and explain what's happening here.
music21/scale/__init__.py
Outdated
| <music21.pitch.Pitch E4> | ||
| >>> sc.getDominant() | ||
| <music21.pitch.Pitch B4> | ||
| >>> sc.pitchFromDegree(1) # scale degree 1 is treated as lowest |
There was a problem hiding this comment.
usually no need for comments in doc tests, write it as documentation just above.
There was a problem hiding this comment.
<music21.pitch.Pitch B4>
>>> sc.pitchFromDegree(1) # scale degree 1 is treated as lowest
<music21.pitch.Pitch E4>
>>> sc = scale.JazzMinorScale()
explain what's happening here.
usually no need for comments in doc tests, write it as documentation just above.
To clarify, these comments are largely borrowed from other diatonic scales (mainly the harmonic minor) that I had used as a template. Should I rework the comments on these other scales as well?
| class AbstractHarmonicMinorScale(AbstractScale): | ||
| ''' | ||
| A true bi-directional scale that with the augmented |
There was a problem hiding this comment.
We're losing two documented classes without a deprecation process. Why are these being removed?
There was a problem hiding this comment.
My thought was that it would be better to be able to create non-natural minor keys in one line, especially considering the fact that in the existing documentation, abstract scales are described as "rarely created or manipulated directly by most users." For example, with the changes in this most recent commit, in order to create a harmonic minor key, users would type:
minorKey = key.Key(mode='harmonic minor')
vs. what was previously required and seems unnecessary:
minorKey = key.Key(mode='minor')
minorKey.abstract = AbstractHarmonicMinor()
Additionally, as far as I can tell, no functionality that a user would manipulate is lost by moving the abstract harmonic/melodic/jazz minor scale classes entirely into the AbstractDiatonicScale class. However, this is my first PR ever; would it be better to leave those abstract classes?
There was a problem hiding this comment.
Smaller PRs are usually better for this very reason. This started as a small feature and bugfix and then added a proposal for a refactor. But that's three motivations, and the bugfix and feature probably could have been merged already were they isolated.
We would ordinarily wait a whole version to deprecate documented classes, with notice on the list and issuing warnings in the code for people still using the old pattern. For such an unclear benefit I'm -1. You can certainly wait to hear from Michael before changing the PR, though.
Thanks for the PR and welcome aboard 👍🏻
There was a problem hiding this comment.
hiya @Phantasmogenesis, absolutely no rush here or anything, and I hope I haven't sounded short, I am genuinely pleased to see more folks lend a hand to m21. It's what I was hoping for when I marked this as a good first issue!
So please do say if you're waiting on anyone to offer guidance or direct any part of the implementation. All the best, and thanks again, it will be great to get JazzMinor and the bugfix you identified into the system. 🥇
| another abstract scale: | ||
|
|
||
| >>> minorKey.abstract = scale.AbstractHarmonicMinorScale() | ||
| >>> minorKey.abstract = key.Key(mode='harmonic minor').abstract |
There was a problem hiding this comment.
This strikes me as a strange way of deriving a scale, i.e., creating a Key object, throwing it away, and then getting the .abstract. Either way we're exposing the user to the concept of abstract scales, so I'm not sure this is an improvement.
|
Hello -- just as a note (from the music21list Google Group) I'm taking a sabbatical from reviewing music21 issues and PRs until at least Jan 1, 2024, so this PR will be deferred until then. |
4 participants
Closes #955 and also allows getRelativeMinor/getRelativeMajor functions to be used on minor scales other than natural minor, as per the documentation.