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.

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

Simplify code and speed up for loops #1631

Merged
merged 6 commits into from
Jan 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
9 changes: 2 additions & 7 deletions music21/analysis/patel.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,8 @@ def melodicIntervalVariability(streamForAnalysis, **skipKeywords):
+ 'a std-deviation of intervals (and thus a MIV)')
# summation = 0
semitoneList = [myInt.chromatic.undirected for myInt in intervalStream]
mean = 0
std = 0
for a in semitoneList:
mean = mean + a
mean = mean / totalElements
for a in semitoneList:
std = std + (a - mean) ** 2
mean = sum(semitoneList) / totalElements
std = sum((a - mean) ** 2 for a in semitoneList)
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
std = math.sqrt(std / (totalElements - 1))
return 100 * (std / mean)

Expand Down
13 changes: 4 additions & 9 deletions music21/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,9 @@ def __eq__(self, other: object):

if len(self) != len(other):
return False
sls = sorted(self)
slo = sorted(other)

for x in range(len(sls)):
if sls[x].lower() != slo[x].lower():
for eSelf, eOther in zip(sorted(self), sorted(other)):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if eSelf.lower() != eOther.lower():
return False
return True

Expand Down Expand Up @@ -3267,9 +3265,7 @@ def splitAtQuarterLength(

elif addTies and isinstance(e, chord.Chord) and isinstance(eRemain, chord.Chord):
# the last isinstance is redundant, but MyPy needs it.
for i in range(len(e.notes)):
component = e.notes[i]
remainComponent = eRemain.notes[i]
for component, remainComponent in zip(e.notes, eRemain.notes):
forceEndTieType = 'stop'
if component.tie is not None:
# the last tie of what was formally a start should
Expand Down Expand Up @@ -3352,8 +3348,7 @@ def splitByQuarterLengths(
eList = []
spannerList = [] # this does not fully work with trills over multiple splits yet.
eRemain = copy.deepcopy(self)
for qlIndex in range(len(quarterLengthList) - 1):
qlSplit = quarterLengthList[qlIndex]
for qlSplit in quarterLengthList[:-1]:
st = eRemain.splitAtQuarterLength(qlSplit,
addTies=addTies,
displayTiedAccidentals=displayTiedAccidentals)
Expand Down
49 changes: 19 additions & 30 deletions music21/beam.py
Original file line number Diff line number Diff line change
Expand Up @@ -328,18 +328,10 @@ def removeSandwichedUnbeamables(beamsList: list[Beams | None]):
None,
None]
'''
beamLast = None
for i in range(len(beamsList)):
Copy link
Member

Choose a reason for hiding this comment

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

This one I'd prefer to leave as is -- the descriptive labels for the names help me understand the logic and I don't feel that two-levels of nested ifs simplify the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you feel about:

for i in range(0, len(beamsList)):
    previousBeamIsNone = (i == 0 or beamsList[i - 1] is None)
    nextBeamIsNone = (i + 1 == len(beamsList) or beamsList[i + 1] is None)
    if previousBeamIsNone and nextBeamIsNone:
        beamsList[i] = None
return beamsList

Copy link
Member

Choose a reason for hiding this comment

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

No, it's still much slower for me to read and not faster for the computer.

if i != len(beamsList) - 1:
beamNext = beamsList[i + 1]
else:
beamNext = None

if beamLast is None and beamNext is None:
for i in range(0, len(beamsList)):
if ((i == 0 or beamsList[i - 1] is None)
and (i + 1 == len(beamsList) or beamsList[i + 1] is None)):
beamsList[i] = None

beamLast = beamsList[i]

return beamsList

@staticmethod
Expand All @@ -350,17 +342,15 @@ def mergeConnectingPartialBeams(beamsList):
16ths are not beamed by default.
'''
# sanitize two partials in a row:
for i in range(len(beamsList) - 1):
bThis = beamsList[i]
bNext = beamsList[i + 1]
for bThis, bNext in zip(beamsList[:-1], beamsList[1:]):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if not bThis or not bNext:
continue

bThisNum = bThis.getNumbers()
if not bThisNum:
continue

for thisNum in bThisNum:
for i, thisNum in enumerate(bThisNum):
thisBeam = bThis.getByNumber(thisNum)
if thisBeam.type != 'partial' or thisBeam.direction != 'right':
continue
Expand All @@ -387,9 +377,7 @@ def mergeConnectingPartialBeams(beamsList):
nextBeam.direction = None

# now fix partial-lefts that follow stops:
for i in range(1, len(beamsList)):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
bThis = beamsList[i]
bPrev = beamsList[i - 1]
for bThis, bPrev in zip(beamsList[1:], beamsList[:-1]):
if not bThis or not bPrev:
continue

Expand All @@ -416,17 +404,17 @@ def mergeConnectingPartialBeams(beamsList):
return beamsList

@staticmethod
def sanitizePartialBeams(beamsList):
def sanitizePartialBeams(beamsList: list[Beams | None]) -> list[Beams | None]:
'''
It is possible at a late stage to have beams that only consist of partials
or beams with a 'start' followed by 'partial/left' or possibly 'stop' followed
by 'partial/right'; beams entirely consisting of partials are removed
and the direction of irrational partials is fixed.
'''
for i in range(len(beamsList)):
if beamsList[i] is None:
for i, beamsObj in enumerate(beamsList):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if beamsObj is None:
continue
allTypes = beamsList[i].getTypes()
allTypes = beamsObj.getTypes()
# clear elements that have partial beams with no full beams:
if 'start' not in allTypes and 'stop' not in allTypes and 'continue' not in allTypes:
# nothing but partials
Expand All @@ -436,7 +424,8 @@ def sanitizePartialBeams(beamsList):
# follow a stop
hasStart = False
hasStop = False
for b in beamsList[i].beamsList:
b: Beam
for b in beamsObj.beamsList:
if b.type == 'start':
hasStart = True
continue
Expand Down Expand Up @@ -583,9 +572,9 @@ def getByNumber(self, number):
'''
if number not in self.getNumbers():
raise IndexError(f'beam number {number} cannot be accessed')
for i in range(len(self)):
if self.beamsList[i].number == number:
return self.beamsList[i]
for beam in self.beamsList:
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if beam.number == number:
return beam

def getNumbers(self):
'''
Expand Down Expand Up @@ -695,10 +684,10 @@ def setByNumber(self, number, type, direction=None): # type is okay @ReservedAs
raise BeamException(f'beam type cannot be {type}')
if number not in self.getNumbers():
raise IndexError(f'beam number {number} cannot be accessed')
for i in range(len(self)):
if self.beamsList[i].number == number:
self.beamsList[i].type = type
self.beamsList[i].direction = direction
for beam in self.beamsList:
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if beam.number == number:
beam.type = type
beam.direction = direction


# -----------------------------------------------------------------------------
Expand Down
51 changes: 21 additions & 30 deletions music21/duration.py
Original file line number Diff line number Diff line change
Expand Up @@ -862,15 +862,11 @@ def ordinal(self):
>>> c.ordinal
14
'''
ordinalFound = None
for i in range(len(ordinalTypeFromNum)):
if self.type == ordinalTypeFromNum[i]:
ordinalFound = i
break
if ordinalFound is None:
try:
return ordinalTypeFromNum.index(self.type)
except ValueError:
raise DurationException(
f'Could not determine durationNumber from {ordinalFound}')
return ordinalFound
f'Could not determine durationNumber from {self.type}')
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved


_durationTupleCacheTypeDots: dict[tuple[str, int], DurationTuple] = {}
Expand Down Expand Up @@ -2135,18 +2131,14 @@ def componentIndexAtQtrPosition(self, quarterPosition):
return self.components[-1]

currentPosition = 0.0
indexFound = None
for i in range(len(self.components)):
currentPosition = opFrac(currentPosition + self.components[i].quarterLength)
for i, component in enumerate(self.components):
currentPosition = opFrac(currentPosition + component.quarterLength)
if currentPosition > quarterPosition:
indexFound = i
break
if indexFound is None:
raise DurationException(
'Could not match quarter length within an index.')
return indexFound
return i
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
raise DurationException(
'Could not match quarter length within an index.')

def componentStartTime(self, componentIndex):
def componentStartTime(self, componentIndex: int) -> float:
'''
For a valid component index value, this returns the quarter note offset
at which that component would start.
Expand All @@ -2172,15 +2164,14 @@ def componentStartTime(self, componentIndex):
IndexError: invalid component index value 3 submitted;
value must be an integer between 0 and 2
'''
if componentIndex not in range(len(self.components)):
raise IndexError(
f'invalid component index value {componentIndex} '
+ f'submitted; value must be an integer between 0 and {len(self.components) - 1}')
if 0 <= componentIndex < len(self.components):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
components = self.components[:componentIndex]
return float(sum([c.quarterLength for c in components]))

raise IndexError(
f'invalid component index value {componentIndex} '
+ f'submitted; value must be an integer between 0 and {len(self.components) - 1}')

currentPosition = 0.0
for i in range(componentIndex):
currentPosition += self.components[i].quarterLength
return currentPosition

def consolidate(self):
'''
Expand Down Expand Up @@ -2624,10 +2615,10 @@ def dotGroups(self, value: tuple[int, ...]):
if not isinstance(value, tuple):
raise TypeError('only tuple dotGroups values can be used with this method.')
# removes dots from all components...
components = list(self._components)
for i in range(len(self._components)):
components[i] = durationTupleFromTypeDots(self._components[i].type, 0)
self._components = tuple(components)
self._components = tuple(
durationTupleFromTypeDots(component.type, 0)
for component in self._components
)
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved

self._dotGroups = value
self._quarterLengthNeedsUpdating = True
Expand Down
11 changes: 6 additions & 5 deletions music21/interval.py
Original file line number Diff line number Diff line change
Expand Up @@ -2560,23 +2560,24 @@ def _stringToDiatonicChromatic(
dirScale = -1
else:
dirScale = 1
value_lower = value.lower()
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved

if 'descending' in value.lower():
if 'descending' in value_lower:
value = re.sub(r'descending\s*', '', value, flags=re.RegexFlag.IGNORECASE)
dirScale = -1
elif 'ascending' in value.lower():
elif 'ascending' in value_lower:
value = re.sub(r'ascending\\s*', '', value, flags=re.RegexFlag.IGNORECASE)

# permit whole and half abbreviations
if value.lower() in ('w', 'whole', 'tone'):
if value_lower in ('w', 'whole', 'tone'):
value = 'M2'
inferred = True
elif value.lower() in ('h', 'half', 'semitone'):
elif value_lower in ('h', 'half', 'semitone'):
value = 'm2'
inferred = True

for i, ordinal in enumerate(common.musicOrdinals):
if ordinal.lower() in value.lower():
if ordinal.lower() in value_lower:
value = re.sub(fr'\s*{ordinal}\s*',
str(i),
value,
Expand Down
36 changes: 10 additions & 26 deletions music21/metadata/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,13 +501,7 @@ def isStandardName(self, name: str) -> bool:
>>> md.isStandardName('soundtracker:SampleName')
False
'''
if self._isStandardNamespaceName(name):
Copy link
Member

Choose a reason for hiding this comment

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

-1 on this. Just hides that it's returning a boolean value and doesn't offer a real speedup. It also makes it harder to add other future tests without encouraging people to make multiline boolean returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does it hide that? I see the second point though.

return True

if self._isStandardUniqueName(name):
return True

return False
return self._isStandardNamespaceName(name) or self._isStandardUniqueName(name)

# -----------------------------------------------------------------------------
# Public APIs
Expand Down Expand Up @@ -1137,7 +1131,7 @@ def search(

# environLocal.printDebug(['comparing fields:', f, field])
# look for partial match in all fields
if field.lower() in uniqueName.lower():
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
if field in uniqueName.lower():
valueFieldPairs.append((value, uniqueName))
match = True
break
Expand All @@ -1148,21 +1142,20 @@ def search(
uniqueName, None
)

if not workId:
if workId is None:
# there is no associated grandfathered workId, don't search it
continue

# look for partial match in all fields
if field.lower() in workId.lower():
if field in workId.lower():
valueFieldPairs.append((value, workId))
match = True
break
else: # get all fields
for uniqueName, value in self.all(skipContributors=True):
if not self._isStandardUniqueName(uniqueName):
# custom metadata, don't search it
continue
valueFieldPairs.append((value, uniqueName))
# only search standard metadata
if self._isStandardUniqueName(uniqueName):
valueFieldPairs.append((value, uniqueName))

# now get all (or field-matched) contributor names, using contrib.role
# as field name, so clients can search by custom contributor role.
Expand Down Expand Up @@ -1969,13 +1962,7 @@ def _isStandardUniqueName(self, uniqueName: str) -> bool:
False

'''
prop: PropertyDescription | None = (
properties.UNIQUE_NAME_TO_PROPERTY_DESCRIPTION.get(uniqueName, None)
)
if prop is None:
return False

return True
return uniqueName in properties.UNIQUE_NAME_TO_PROPERTY_DESCRIPTION
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved

@staticmethod
def _isStandardNamespaceName(namespaceName: str) -> bool:
Expand Down Expand Up @@ -2789,11 +2776,8 @@ def _isStandardUniqueName(self, uniqueName: str) -> bool:
>>> rmd._isStandardUniqueName('average duration')
False
'''
if super()._isStandardUniqueName(uniqueName):
return True
if uniqueName in self.additionalRichMetadataAttributes:
return True
return False
return (super()._isStandardUniqueName(uniqueName)
Copy link
Member

Choose a reason for hiding this comment

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

-1 as above -- I generally prefer the expanded way unless there's a real space/mind/memory savings from returning the value of a boolean expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain why? To me the short boolean expression would seem much easier to parse/understand quickly.

Copy link
Member

Choose a reason for hiding this comment

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

It's much harder for me to keep track of why we're returning what we're returning and it's really not a speed improvement in the system. In the old system we test one thing and if it's good, we return True, and then we test another thing, and if it's good, we return True. Then if we get to the end, we return False. Very easy to look at the logic. In the new way, I have to look ahead and see "is there going to be an and clause? Are we going to add the values and do something with it? Are the parentheses there for a reason or only because the line is too long?"

It just makes me read code slower, and there's no discernible speedup.

or uniqueName in self.additionalRichMetadataAttributes)


# -----------------------------------------------------------------------------
Expand Down
3 changes: 1 addition & 2 deletions music21/metadata/caching.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,12 +356,11 @@ def process_parallel(jobs, processCount=None):
# end generator

@staticmethod
def process_serial(jobs):
def process_serial(jobs: list[MetadataCachingJob]):
'''
Process jobs serially.
'''
remainingJobs = len(jobs)
results = []
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
for job in jobs:
results, errors = job.run()
remainingJobs -= 1
Expand Down
6 changes: 3 additions & 3 deletions music21/metadata/primitives.py
Original file line number Diff line number Diff line change
Expand Up @@ -862,10 +862,10 @@ def __init__(self,
def __str__(self):
if isinstance(self._data, bytes):
return self._data.decode('UTF-8')
elif not isinstance(self._data, str):
return str(self._data)
else:
elif isinstance(self._data, str):
TimFelixBeyer marked this conversation as resolved.
Show resolved Hide resolved
return self._data
else:
return str(self._data)

def _reprInternal(self):
return str(self)
Expand Down
Loading