Fix #1653: Correct scale caching mechanism. #1692
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We cannot use the name of a ConcreteScale to cache the scales as there are sometimes different scales with the same name. Using the pitch list and names now to be more precise and avoid collisions (cuthbertLab#1653).
mscuthbert
reviewed
Apr 25, 2024
There was a problem hiding this comment.
Thanks. Good catch. I ran tests here myself, but try to make sure when making changes to a caching system (which is generally there for necessity) that the use of the cache is still tested (generally with timing tests to back it up). This slows down the creation of roman numerals by 8%, which is acceptable given the bug it fixes. If it slowed it down by 200% then we would need to make some other fixes (such as id hashing, etc.)
| if not self.useImpliedScale: | ||
| useScale = self._scale | ||
| else: | ||
| if self.useImpliedScale: |
There was a problem hiding this comment.
perhaps a bit more elegant, but this change is not really in the scope of the PR. :-)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1653: We cannot use the name of a
ConcreteScaleto cache the scales as there are sometimes different scales with the same name but different pitches. Using the pitch list and names now to be more precise and avoid collisions.Also adds a regression test. (and a small unrelated simplification).