Repository navigation
Increase speed of Fractions.limit_denominator #93476
Description
Activity
- added a commit that references this issue
on Jun 3, 2022 In this case, we value algorithmic clarity over a minor performance gain from inlining.
Also, we would lose some of the recent optimizations in the Fraction class.
The limit_denominator code is almost never in a performance critical section of code, so no one would notice the small benefit, but we would incur a loss of understanding about how it works.
Thanks for the suggestion, but we will decline this one.
Feature or enhancement
Speed up Fractions.limit_denominator by not creating intermediary Fraction objects
Pitch
Fractions.limit_denominatorreturns a new Fraction, but along the way for Fractions > max_denominator creates 3 Fraction objects that are discarded: the fraction farther from the bound and two temporary fractions (bound1 - selfandbound2 - shelf) which are created just for getting their absolute values.Similar code speedups have been part of the
music21library since 2015--where we call limit_denominator after every arithmetic operation on note placement--and timing tests give a 7.3x speedup. (see music21 Source )For fractions with denominators equal to or below max_denominator, instantiate the new Fraction with
Fraction(self._numerator, self._denominator)instead ofFraction(self)for a 23% speedup.PR to follow.