Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented jazz minor scale and relative key functionality on harmonic, melodic, and jazz minors #1044

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+55 −68
Open

Implemented jazz minor scale and relative key functionality on harmonic, melodic, and jazz minors #1044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c6a6d6
Implemented jazz minor scale and expanded relative key functionality
Phantasmogenesis Jun 25, 2021
83498f0
Update __init__.py
Phantasmogenesis Jun 25, 2021
6b88cbb
Implemented all minor scales as DiatonicScales and fixed problematic …
Phantasmogenesis Jun 26, 2021
48c3695
Fixed harmonic minor scale and changed abstract scale usage
Phantasmogenesis Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion music21/key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,7 @@ def deriveByDegree(self, degree, pitchRef):
To use the harmonic form, change `.abstract` on the key to
another abstract scale:

>>> minorKey.abstract = scale.AbstractHarmonicMinorScale()
>>> minorKey.abstract = key.Key(mode='harmonic minor').abstract
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

>>> minorKey.deriveByDegree(7, 'E')
<music21.key.Key of f minor>
>>> minorKey.deriveByDegree(6, 'G')
Expand Down
121 changes: 54 additions & 67 deletions music21/scale/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,13 @@
'TERMINUS_LOW', 'TERMINUS_HIGH',
'ScaleException', 'Scale',
'AbstractScale', 'AbstractDiatonicScale', 'AbstractOctatonicScale',
'AbstractHarmonicMinorScale', 'AbstractMelodicMinorScale',
'AbstractCyclicalScale', 'AbstractOctaveRepeatingScale',
'AbstractRagAsawari', 'AbstractRagMarwa', 'AbstractWeightedHexatonicBlues',
'ConcreteScale', 'DiatonicScale', 'MajorScale',
'MinorScale', 'DorianScale', 'PhrygianScale', 'LydianScale', 'MixolydianScale',
'HypodorianScale', 'HypophrygianScale', 'HypolydianScale', 'HypomixolydianScale',
'LocrianScale', 'HypolocrianScale', 'HypoaeolianScale',
'HarmonicMinorScale', 'MelodicMinorScale',
'HarmonicMinorScale', 'JazzMinorScale', 'MelodicMinorScale',
'OctatonicScale', 'OctaveRepeatingScale', 'CyclicalScale', 'ChromaticScale',
'WholeToneScale', 'SieveScale', 'ScalaScale', 'RagAsawari',
'RagMarwa', 'WeightedHexatonicBlues',
Expand Down Expand Up @@ -752,6 +751,22 @@ def buildNetwork(self, mode=None):
intervalList = srcList[5:] + srcList[:5] # a to A
self.relativeMajorDegree = 3
self.relativeMinorDegree = 1
elif mode == 'harmonic minor':
intervalList = ['M2', 'm2', 'M2', 'M2', 'm2', 'A2', 'm2'] # a to A
self.relativeMajorDegree = 3
self.relativeMinorDegree = 1
elif mode == 'melodic minor':
self._net = intervalNetwork.IntervalNetwork(
octaveDuplicating=self.octaveDuplicating,
pitchSimplification=None)
self.relativeMajorDegree = 3
self.relativeMinorDegree = 1
self._net.fillMelodicMinor()
return
elif mode == 'jazz minor':
intervalList = ['M2', 'm2', 'M2', 'M2', 'M2', 'M2', 'm2'] # a to A
self.relativeMajorDegree = 3
self.relativeMinorDegree = 1
elif mode == 'locrian':
intervalList = srcList[6:] + srcList[:6] # b to B
self.relativeMajorDegree = 2
Expand Down Expand Up @@ -842,58 +857,6 @@ def buildNetwork(self, mode=None):
# might also set weights for tonic and dominant here


class AbstractHarmonicMinorScale(AbstractScale):
'''
A true bi-directional scale that with the augmented
Comment on lines -845 to -847
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're losing two documented classes without a deprecation process. Why are these being removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jacobtylerwalls jacobtylerwalls Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🥇

second to a leading tone.

This is the only scale to use the "_alteredDegrees" property.
'''

def __init__(self, mode=None):
super().__init__()
self.type = 'Abstract Harmonic Minor'
self.octaveDuplicating = True
self.dominantDegree: int = -1
self.buildNetwork()

def buildNetwork(self):
intervalList = ['M2', 'm2', 'M2', 'M2', 'm2', 'M2', 'M2'] # a to A
self.tonicDegree = 1
self.dominantDegree = 5
self._net = intervalNetwork.IntervalNetwork(intervalList,
octaveDuplicating=self.octaveDuplicating,
pitchSimplification=None)

# raise the seventh in all directions
# 7 here is scale step/degree, not node id
self._alteredDegrees[7] = {
'direction': intervalNetwork.DIRECTION_BI,
'interval': interval.Interval('a1')
}


class AbstractMelodicMinorScale(AbstractScale):
'''
A directional scale.
'''

def __init__(self, mode=None):
super().__init__()
self.type = 'Abstract Melodic Minor'
self.octaveDuplicating = True
self.dominantDegree: int = -1
self.buildNetwork()

def buildNetwork(self):
self.tonicDegree = 1
self.dominantDegree = 5
self._net = intervalNetwork.IntervalNetwork(
octaveDuplicating=self.octaveDuplicating,
pitchSimplification=None)
self._net.fillMelodicMinor()


class AbstractCyclicalScale(AbstractScale):
'''
A scale of any size built with an interval list of any form.
Expand Down Expand Up @@ -2729,7 +2692,7 @@ class HypophrygianScale(DiatonicScale):
<music21.pitch.Pitch E4>
>>> sc.getDominant()
<music21.pitch.Pitch A4>
>>> sc.pitchFromDegree(1) # scale degree 1 is treated as lowest
>>> sc.pitchFromDegree(1)
<music21.pitch.Pitch B3>
'''
def __init__(self, tonic=None):
Expand Down Expand Up @@ -2830,9 +2793,9 @@ class HarmonicMinorScale(DiatonicScale):
'''
The harmonic minor collection, realized as a scale.

(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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the space, but the "but" should remain

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, because "where" is not delimiting certain parts of the world where the term is/is not meaningful.

world, where the term "Harmonic Minor" would not be appropriate).

>>> sc = scale.HarmonicMinorScale('e4')
>>> [str(p) for p in sc.pitches]
Expand All @@ -2841,7 +2804,7 @@ class HarmonicMinorScale(DiatonicScale):
<music21.pitch.Pitch E4>
>>> sc.getDominant()
<music21.pitch.Pitch B4>
>>> sc.pitchFromDegree(1) # scale degree 1 is treated as lowest
>>> sc.pitchFromDegree(1)
<music21.pitch.Pitch E4>

>>> sc = scale.HarmonicMinorScale()
Expand All @@ -2856,12 +2819,7 @@ def __init__(self, tonic=None):
super().__init__(tonic=tonic)
self.type = 'harmonic minor'

# note: this changes the previously assigned AbstractDiatonicScale
# from the DiatonicScale base class

self._abstract = AbstractHarmonicMinorScale()
# network building happens on object creation
# self._abstract.buildNetwork()
self._abstract.buildNetwork(self.type)


class MelodicMinorScale(DiatonicScale):
Expand All @@ -2875,9 +2833,38 @@ def __init__(self, tonic=None):
super().__init__(tonic=tonic)
self.type = 'melodic minor'

# note: this changes the previously assigned AbstractDiatonicScale
# from the DiatonicScale base class
self._abstract = AbstractMelodicMinorScale()
self._abstract.buildNetwork(self.type)


class JazzMinorScale(DiatonicScale):
'''
The jazz minor scale.
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

A synthetic scale identical to the ascending melodic minor. Useful in
jazz, especially over harmonies that employ minor-major seventh chords.

>>> sc = scale.JazzMinorScale('e4')
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
>>> [str(p) for p in sc.pitches]
['E4', 'F#4', 'G4', 'A4', 'B4', 'C#5', 'D#5', 'E5']
>>> sc.getTonic()
<music21.pitch.Pitch E4>
>>> sc.getDominant()
<music21.pitch.Pitch B4>
>>> sc.pitchFromDegree(1)
<music21.pitch.Pitch E4>
>>> sc = scale.JazzMinorScale()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space above and explain what's happening here.

>>> sc.deriveRanked(['C', 'E', 'G'], comparisonAttribute='name')
[(3, <music21.scale.JazzMinorScale G jazz minor>),
(3, <music21.scale.JazzMinorScale F jazz minor>),
(2, <music21.scale.JazzMinorScale B- jazz minor>),
(2, <music21.scale.JazzMinorScale A jazz minor>)]
'''

def __init__(self, tonic=None):
super().__init__(tonic=tonic)
self.type = 'jazz minor'

self._abstract.buildNetwork(self.type)


# ------------------------------------------------------------------------------
Expand Down