Classlib: Patterns: Pkey embedInStream must provide a default repeats

[jamshark70]

Without the fix:

(
p = Pbind(
	\stuff, Pwhite(30, 36, inf),
	\midinote, Pseq([Pkey(\stuff), 62, 64, 65], inf)
).asStream;
)

4.do { p.next(()).postln };
( 'stuff': 34, 'midinote': 62 )
( 'stuff': 35, 'midinote': 64 )
( 'stuff': 36, 'midinote': 65 )
( 'stuff': 31, 'midinote': 62 )

Pkey is skipped because: repeats.value(inval).do but the default repeats is nil 🤦‍♂️

We should substitute inf repeats -- Pkey with no repeats specified should be infinite-length. In fact, I had suggested this at one point, but then somebody removed the nil check.

I think we shouldn't assign a default in the class. Currently we're using nil as an optimization, to avoid checking the stream length if it's going to be infinite anyway.

[telephon]

It is good to distinguish between asStream (inf) and embedInStream (1).

We really should have a number of tests that unfold the expected behaviors.

I've started writing some lines of code for that already, but maybe you have ideas what kind of contexts we should take into account:

Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pkey(\x))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pkey(\x, inf))).asStream.nextN(4)


Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pseq([Pkey(\x)], inf))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pseq([Pkey(\x, inf)], inf))).asStream.nextN(4)


Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pseq([Pkey(\x, 1)], 1))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \y, Pkey(\x, 1))).asStream.nextN(4)


///////// pattern for keys



Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pkey(Pseq([\x, \u], 1)))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pkey(\x, inf))).asStream.nextN(4)


Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pseq([Pkey(Pseq([\x, \u], 1))], inf))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pseq([Pkey(\x, inf)], inf))).asStream.nextN(4)


Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pseq([Pkey(Pseq([\x, \u], 1), 1)], 1))).asStream.nextN(4)
Pevent(Pbind(\x, Pseq([1, 2, 3], inf), \u, 8, \y, Pkey(Pseq([\x, \u], 1), 1))).asStream.nextN(4)

[telephon]

After some thought, I am quite sure it should be

repeats.value(inval) ? 1

[jamshark70]

I've explained my reasoning in the main thread -- in short, this looks innocent enough, but it sets a precedent for a large semantic change for patterns. I don't think we should do it casually, no matter how convenient it might be in this one case.

[telephon]

agreed.
Are you sure that it isn't faster to do
(repeats.value(inval) ? inf ).do {
here?

[jamshark70]

I'm pretty sure we already had this conversation. I believe it's categorically impossible for a push + method call every time (?) to be faster than a jump + only sometimes push (?? {}). I'll benchmark again when I'm at my desk but I'm pretty sure it will confirm.

[jamshark70]

Results just posted to the mailing list. ?? is definitely faster. You might have had an outlier result once upon a time, but it doesn't hold up to repeated tests.

[jamshark70]

It is good to distinguish between asStream (inf) and embedInStream (1).

That, right there, should be a red flag to tread carefully -- making it exactly the sort of case where my push to draw up specs is completely relevant.

For years, embedInStream has defined patterns' behavior, and asStream is merely a Routine wrapper for that. (There are a few FuncStream exceptions, but not many.) We have been able to assume that embedInStream and asStream will produce the same streaming behavior.

To make a distinction between these two is a significant change of interface -- a breaking change. I would even go so far as to say that it alters the fundamental contract:

• Currently, each pattern argument has one and only one default value, and that value behaves consistently everywhere the pattern is used. Prand(array) always and every time produces one value and stops, even though this may be counterintuitive in the case of Pbind(\dur, Prand(array)). There is a simple principle (defaults behave consistently), and this is easy to explain to users, and users should write what they want (if they need an infinite Prand stream, add inf).

• The proposal is that patterns' repeats arguments modify their behavior depending on context.

  • Pro: Patterns might (in most cases?) behave more intuitively with less user input. (edit: "Might"... "in most cases"... more red flags. Specs would answer these questions.)
  • Cons:
    • It's a more complex principle that is harder to explain to users, and users need to understand more background details in order to know what to expect from their code. (And, if it's implemented inconsistently, forget about users understanding anything...)
    • We need to be absolutely clear when to use asStream vs embedInStream (as this is a new distinction, I don't think we're clear on this at all), and we need a code review to make sure the class library isn't using either one inappropriately.

I've reread #2411 and I do understand the rationale for considering this -- and in fact, I don't necessarily disagree with it, and I'm willing to discuss it. But to change Pkey and only Pkey in a way that violates an at-least decade-long basic rule of patterns, without a spec, without considering what it means for other patterns... no, I'm afraid I can't accept that just yet.

You might be right that eventually we will need to change Pkey:embedInStream to default to 1 repeat. I think it's premature to do that today. I think this PR should just restore backward compatibility, and we discuss the rest elsewhere.

[jamshark70]

And I realized later, #2412 is (currently) a large nail in that coffin.

(
p = Pbind(
	\stuff, Pwhite(30, 36, inf),
	\midinote, Pseq([Pkey(\stuff), Pkey(\stuff) + 10, 62, 64, 65], inf)
).asStream;
)

If the default embedInStream behavior is to embed one value, then you'll still get stuck on the binary op. So we have inconsistent behavior even within the same list pattern. Good luck explaining that in the help...

We need to build the infrastructure to support awareness of the streaming context before changing stuff for the sake of localized convenience. Then we can come back to talk about Pkey embedding.

[telephon]

Pkey is a pattern that looks up values. You can use a pattern of keys. So if you like, the pattern of keys is the essential point whose semantics needs to be decided first.

The main difficulty is there that a pattern might also be a value (usually a symbol in this case). This is similar to Pbind:

Pbind(
   \freq, 500, // value
)

Pbind(
   \freq, Pseq([100, 200, 140]) // pattern
)

// analogously:

Pkey(
   \x  // value
)
 
Pkey(
   Pseq([\x, \y, \x]) // pattern
)

In a Pbind, a single value loops, but a pattern is embedded only once. The point is in the distinction between asStream and embedInStream, which behave the same for patterns, but -- and this is essential -- differently for Object:

Object {
   	embedInStream { ^this.yield }
        asStream { ^this }
}

Btw. I had the difficulty in Pdefn of how to represent this difference, because Pdefn should transparently represent the pattern (or object) that it contains. The solution is then to maintain the distinction by passing streamArg(embed:true), or streamArg(embed:false) respectively, to the inner pattern.

We may not need that message, because we have separate asStream and embedInStream methods in Pkey.

[telephon]

I've reread #2411 and I do understand the rationale for considering this -- and in fact, I don't necessarily disagree with it, and I'm willing to discuss it. But to change Pkey and only Pkey in a way that violates an at-least decade-long basic rule of patterns, without a spec, without considering what it means for other patterns... no, I'm afraid I can't accept that just yet.

I agree, we should get this clear. Note however that in OOP, behavior is always bound to objects, so it is typical that such discussions must appear at some point. In this case, we have teh example of Pbind that probably attuned certain expectations.

[jamshark70]

We may not need that message, because we have separate asStream and embedInStream methods in Pkey.

As you noted in another issue, this will not help when a math operator is applied, because the P*op pattern always uses asStream, no matter the calling context. I expect the same issue for filter patterns.

That's what I mean when I say we don't have the infrastructure to do this properly yet. Until we do have it, trying to use these two methods to distinguish the cases is a clever idea, in the same way that my original Rest concept was a clever idea (which turned out to be a ghastly mess). Let's learn from past mistakes instead of repeating them.

[telephon]

ah yes.

Pdefn(\z, Pdefn(\x, 2) + Pdefn(\y, 3));
Pseq([Pdefn(\z), 100], inf).asStream.nextN(4) // [ 5, 5, 5, 5 ] instead of  [ 5, 100, 5, 100 ] 

Good, so let's see if we can find a way to define the conditions for the behaviours.

[jamshark70]

I still feel quite strongly that we should merge this PR fairly quickly, to correct the regression issue introduced by the Pkey repeats fix. Regression = bad, we shouldn't delay.

We can use #2412 for the larger discussion.

[mossheim]

I still feel quite strongly that we should merge this PR fairly quickly, to correct the regression issue introduced by the Pkey repeats fix. Regression = bad, we shouldn't delay.

Does this PR accomplish that right now, or does it still need work? I am in favor of correcting the regression for now since it seems like this still requires more discussion.

[jamshark70]

Does this PR accomplish that right now...?

I believe it does.

• The old behavior is that embedding a Pkey in a stream would repeat infinitely.

• The current behavior is that a Pkey that doesn't specify a number of repeats is omitted when embedding.

• After this fix, a Pkey that doesn't specify a number of repeats will repeat indefinitely when embedded (the old behavior).

Side note, it took 3 minutes to propose this one line change, and hours to defend it. Review is good, but this is a little out of control.

[telephon]

Side note, it took 3 minutes to propose this one line change, and hours to defend it. Review is good, but this is a little out of control.

It also took hours to review it, even if it took only 3 minutes to propose it. These things are not isolated. Also no need to defend it, I just try to contribute to common understanding.

Side note: it had positive side effects.

[jamshark70]

Side note: it had positive side effects.

It did, and I'm not saying it shouldn't be done. This was, however, a bit of a perfect storm: I added two semantic units, both of them were questioned, and even just to explain why I wrote the operator and operand that I did involves a lot of backstory. So the effect is a considerable burden on me, for technical reasons that were not convincing to me.

Perhaps, for regression fixes, review could focus on whether the fix adequately and correctly restores the pre-regression behavior.

Of "backstory," my workplace is choking us to death with paperwork and micromanagement under the guise of reviews. I'm feeling lately like I have to justify which nostril I breathe through. While I agree with the principle behind review, I hope contributing a quick fix to SC doesn't always have to be quite this involved. If it does, until work lightens up, I will start thinking twice about submitting PRs (is it worth it, or better just to log a bug and create work for someone else?). There must be a middle ground where we ensure code quality without alienating long-term developers.

[telephon]

Oh dear, yes. Review is a very dangerous thing indeed. And it is good you remind us that this is not only true in university but also in places like here.

Sorry for the noise, anyway!

[mossheim]

@telephon - what are your feelings on this PR? You still have an outstanding 'request changes' review, and I can't tell if that should be dismissed and this PR merged or not.