Simplify code and speed up for loops #1631
Conversation
Simplifies various code without changing functionality
TimFelixBeyer
changed the title
music21/beam.py
Outdated
| beamLast = None | ||
| for i in range(len(beamsList)): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 beamsListThere was a problem hiding this comment.
No, it's still much slower for me to read and not faster for the computer.
There was a problem hiding this comment.
Hi -- thanks for the changes. I'm going to make the changes back in the three places I still want them reverted and then merge -- I'm only back a few days a month and this PR is taking too much of my mind space to reread when I do. Feel free after I merge to keep arguing for why I'm doing something inefficient or old-way and then if you convince me, make a followup PR on them, but even approahing those lines with a fresh set of eyes months later I still couldn't quickly understand them until I read the code they were replacing.
music21/beam.py
Outdated
| beamLast = None | ||
| for i in range(len(beamsList)): |
There was a problem hiding this comment.
No, it's still much slower for me to read and not faster for the computer.
music21/metadata/__init__.py
Outdated
| if uniqueName in self.additionalRichMetadataAttributes: | ||
| return True | ||
| return False | ||
| return (super()._isStandardUniqueName(uniqueName) |
There was a problem hiding this comment.
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.
music21/musicxml/xmlToM21.py
Outdated
| for step, alter, accidental in zip(steps, alters, accidentals): | ||
| p = pitch.Pitch(step) | ||
| if accidental is not None: | ||
| accidentalName = self.mxAccidentalNameToM21.get(accidental, accidental) |
There was a problem hiding this comment.
It doesn't seem obvious to me that container.get(thing, thing) is a clear way of saying that if the thing doesn't have a translation in the container to leave it alone.
It's a code style in music21 and I'm the one who will be living with it for the next 20 years likely.
| for i in range(len(mLists) - 1): | ||
| if len(mLists[i]) != len(mLists[i + 1]): | ||
| for mThis, mNext in zip(mLists, mLists[1:]): | ||
| if len(mThis) != len(mNext): | ||
| raise UnequalPartsLengthException( |
| for i in range(len(mLists) - 1): | ||
| if len(mLists[i]) != len(mLists[i + 1]): | ||
| for mThis, mNext in zip(mLists, mLists[1:]): | ||
| if len(mThis) != len(mNext): | ||
| raise UnequalPartsLengthException( |
Opening this PR now so I don't forget - enjoy the sabbatical!
This PR is improves readability and speed throughout
music21.It looks fairly messy but all the changes are quite straightforward and do not affect any functionality.
Many changes are of this type:
to the more pythonic
In the process, a bunch of other small functions were simplified drastically as well.
Happy to break it up into smaller PR's if required as well.