Clear cache when adding notes w/o sort #1357
Conversation
Wrong clients and chordAttached
|
Requesting review from @jacobtylerwalls -- I think this is an improvement, but it makes one thing cached (chord's sort status) that requires better informing from pitches and notes when one of them changes. It might need some more cases. |
There was a problem hiding this comment.
Great, this is an improvement. I see why you wanted extra eyes.
Existing issue -- only one client is possible, so you can have situations like this. Not for this PR, but just mentioning:
>>> n = note.Note('F')
>>> c1 = chord.Chord([n])
>>> c2 = chord.Chord([n])
>>> c1.sortAscending(inPlace=True)
>>> c2.sortAscending(inPlace=True)
>>> n.transpose(6, inPlace=True)
>>> c2._cache
{}
>>> c1._cache
{'isSortedAscendingDiatonic': True}
music21/stream/base.py
Outdated
| elif '_offsetDict' in self.__dict__: | ||
| newOffsetDict2: t.Dict[int, t.Tuple[OffsetQLSpecial, base.Music21Object]] = {} | ||
| setattr(new, '_offsetDict', newOffsetDict2) |
There was a problem hiding this comment.
This is trivial, but do you want to take the opportunity to simplify this? AFAICT _offsetDict in self.__dict__ will always be true, so this case and the case above amount to just self._offsetDict = {}.
music21/pitch.py
Outdated
| self._client: t.Optional['Pitch'] = None | ||
| self.client: t.Optional['music21.note.Note'] = None |
There was a problem hiding this comment.
This and the one below for Pitch is worth calling out in the release notes. I have code that would break if _chordAttached became public, and so I figure the same could apply here. I know private stuff can break at any time, so I'm not suggesting a deprecation cycle, but at the least just calling out in the release.
There was a problem hiding this comment.
_client will remain private.
There was a problem hiding this comment.
And ACK -- that was right before.
| @@ -3989,6 +4025,7 @@ def semiClosedPosition(self, *, forceOctave=None, inPlace=False, leaveRedundantP | |||
| newRemainingPitches.append(p) | |||
| remainingPitches = newRemainingPitches | |||
|
|
|||
| c2.clearCache() | |||
There was a problem hiding this comment.
The octave setter informs client, so I think you're okay--but belt and suspenders is fine.
| @@ -2863,6 +2872,7 @@ def step(self, usrStr: StepName) -> None: | |||
| self.spellingIsInferred = False | |||
| else: | |||
| raise PitchException(f'Cannot make a step out of {usrStr!r}') | |||
| self.informClient() | |||
|
you're right -- maybe better to keep |
oh... that is a thing. Need to consider a bit more. The original plan when Chris Ariza and I designed the sites system was that things like ._chordAttached would be a site for an object -- that sites wouldn't just be Streams that they were in but other contexts, including things like interpretive elements (tuning systems, etc.) --so that there were sites and "locations" (Streams) which are the only things (besides The same thing about having only one client for an object goes with Pitch objects as well. |
Should go back to bed.
This was failing:
The cache was only being cleared during
runSort=True, which is the default. But needs to happen either side.Also caches the sort status, so repeated Chord.sort(inPlace=True) should be much faster.