Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rest handling with patterns #4

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jamshark70
Copy link

@jamshark70 jamshark70 commented Nov 1, 2019

Summary

Currently it is easy to embed a rest of a fixed value in a sequence: Pseq([0, Rest(1), 2]).

It is not easy to embed a rest with being lazy-calculation value: Pseq([0, Pwhite(1, 3, 1).collect(Rest(_)), 2]).

Motivation

This question keeps coming up repeatedly on the mailing list and forum. It's a legitimate user need.

Users often expect to be able to write Rest(Pwhite(1, 3)) and have the Rest be automatically stream-ified. This might be a reasonable expectation (and not hard to implement). But there is also precedent to reject that approach, e.g., an array of patterns requires a Ptuple() wrapper.

Alternately, we could follow the model of .collect -> Pcollect, .select -> Pselect, .keep -> Pfin and provide a .rest method for patterns and other objects.

Some prior discussion is here.

https://scsynth.org/t/pbind-and-samples-with-different-durations-2/735/13

Specification

  1. A Prest(patternOrStream, n) pattern, to take n values from the source pattern and output Rest() objects with those values -- following the model of Pcollect to implement .collect, etc.
    a. Default n should be 1. See unresolved questions.

  2. A .rest(n) method that returns Prest(this, n) for patterns, and Rest(this) for other objects. Possibly, for SequenceableCollections, we might do this.collect(Rest(_)).

    • 2.rest --> Rest(2)
    • Pwhite(1, 3, inf) --> Prest(Pwhite(1, 3, inf), 1)
    • (Maybe?) [1, 2].rest --> [Rest(1), Rest(2)] but here, I don't have a concrete use case. Currently the relationship between rests and arrays is undefined.

Drawbacks

A possible objection is "too many ways to write the same thing." In fact, I raised that objection to a .rest method in the past. But we have both .fin and .keep as pattern aliases for Pfin, so there is precedent and I've relaxed my objection.

Unresolved Questions

.rest is not precisely analogous to .collect.

In pattern.collect(...), we expect every value of the pattern to be streamed out and further mapped by the function.

In pattern.rest(...), it's pointless to stream out all the values and turn them into rests, because then you will have a potentially long series of silent events.

Pbind(\degree, Pwhite(0, 7, inf), \dur, Pseq([0.5, 0.5, rest(Pwhite(1, 4, inf) * 0.5)], inf)) -- if we follow the collect model, then you get two notes and then silence, indefinitely.

I believe the best solution is to limit the length of the Prest stream, default = 1. But this is worth discussing.

(xyz).rest vs rest(xyz) vs Rest(xyz)

Note here that I decided to save a dot by using function-style notation: rest(Pwhite(1, 4, inf) * 0.5).

If we support this, then inevitably -- guaranteed -- some user will write Rest(Pwhite(1, 4, inf) * 0.5) and get annoyed that it doesn't work.

I think it wouldn't be hard to fold Prest behavior into Rest:embedInStream. Is this worth doing for user friendliness, or should we be strict and enforce the distinction between a Rest object (simple value) and the rest method applied to patterns?

@mossheim
Copy link
Contributor

mossheim commented Nov 1, 2019

@jamshark70 looks like you committed two files by mistake, can you please remove 0000-....md?

@jamshark70 jamshark70 force-pushed the rfc/hjh-rest-handling branch from c0808ff to 902f31e Compare November 1, 2019 07:18
@jamshark70
Copy link
Author

OK... I had renamed using "git mv" but then didn't load the new file in the editor. Dumb mistake.

@telephon
Copy link
Member

telephon commented Nov 1, 2019

I think it wouldn't be hard to fold Prest behavior into Rest:embedInStream. Is this worth doing for user friendliness, or should we be strict and enforce the distinction between a Rest object (simple value) and the rest method applied to patterns?

I tend not to fold it into, unless we find a really clear way to make it understandable. We already do a special case for booleans, but that is a very different behavior and use case.

.rest is not precisely analogous to .collect.

I think it should be analogous. You may want to create an infinite pattern of rests and then interlace it with another non rest pattern.

@jamshark70 jamshark70 changed the title Submit RFC for rest handling in patterns Rest handling with patterns Nov 2, 2019
@jamshark70
Copy link
Author

jamshark70 commented Nov 2, 2019

I think it should be analogous. You may want to create an infinite pattern of rests and then interlace it with another non rest pattern.

This, at least, is already handled in the proposal:

Ppatlace([
	Pwhite(0, 7, inf),
	Prest(
		Pwhite(1, 4, inf) * 0.25,
		inf  // <-- HERE, tell Prest to keep going forever
	)
], inf).asStream.nextN(6)

-> [ 0, Rest(0.25), 1, Rest(0.25), 6, Rest(0.75) ]

I'm arguing that this is not the normal case, and based on forum and mailing list questions, users will tend to expect rests to interlace, rather than embed. So we should make the typically expected case into the default.

I tend not to fold it into, unless we find a really clear way to make it understandable.

It certainly needs some thinking-through. I can't do it just at the moment, but I can say for now that "folding" is the most risky and also the least critical element of the proposal. We could drop it easily.

@jamshark70
Copy link
Author

jamshark70 commented Nov 2, 2019

For quick reading, I'll put the conclusion first: With some testing, I can't find a major technical objection to allowing Rest object to handle patterns/streams internally. All of the cases just work.

The question is: Rests are currently stateless. If we go with what's below, they become potentially stateful. That's a big change and I wouldn't do it casually -- definitely needs some scrutiny. (E.g. now you can do r = Rest(2) and you can put r anywhere in a pattern, and it behaves the same. That wouldn't be true if we change it as below and r = Rest(Pwhite(...)).)

I'm of two minds. On the one hand, "one more way to do things" carries some risk. On the other, I don't like unnecessary roadblocks for users' creative workflow. From that perspective, supporting rest(pattern) but not supporting Rest(pattern) could be seen as needlessly pedantic.

Tests:

To make things more concrete: this is what it would look like if Rest automatically handled embedded patterns:

Rest : Operand {
	var stream, <>repeats = 1;

	*new { |value = 1|
		^super.new.value_(value.dereferenceOperand).unwrapBoolean
	}

	value_ { |val|
		value = val;
		stream = value.asStream;
	}

	dur_ { |dt| this.value = dt }
	dur { ^value.value }

	embedInStream { |inval|
		var out;
		repeats.do {
			out = stream.next(inval);

			// nil values from the stream should go out directly
			// so, the end of the rest stream stops the calling pattern
			// (compatible with nil handling everywhere else)
			if(out.notNil) {
				out = Rest(out);
			};
			inval = out.yield;
		}
		^inval
	}

	// ... remainder of the class as is
}

Simple regression (constant value) -- no change in behavior.

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(0.125)], inf)
).play;
)

p.stop;

Infinite-length pattern laces one value and cedes control:

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pwhite(1, 4, inf).trace * 0.125)], inf)
).play;
)

p.stop;

If the Rest object's stream comes to an end, it stops the calling pattern (as if any other pattern had returned nil).

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pseries(1, 1, 4).trace * 0.125)], inf)
).play;
)

p.stop;

One nice thing is, because Rest and Pattern both handle math operators, it doesn't matter where you put the *: Rest(pattern * something) and Rest(pattern) * something both become Rest(Pbinop('*', pattern, something)) and the behavior is the same. I didn't need to make any logic changes to support that.

(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Rest(Pseries(1, 1, 4).trace) * 0.125], inf)
).play;
)

p.stop;

By contrast, .rest and Prest don't handle this quite as cleanly. Because Prest starts from the beginning every time it's embedded, the Pseries starts from the beginning as well. It's technically correct justifiable and matches other behaviors in the pattern system, but it isn't optimally useful.

// rest is always 1 * 0.125
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), (Pseries(1, 1, 4).trace * 0.125).rest], inf)
).play;
)

p.stop;

// you have to add .asStream
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), (Pseries(1, 1, 4).asStream.trace * 0.125).rest], inf)
).play;
)

p.stop;

@telephon
Copy link
Member

telephon commented Nov 2, 2019

I find it a bit strange that a Rest converts a pattern to a stream internally, making it stateful.

The functionality of partial embedding a pattern seems to be a more generally useful one, independent of Rest. Then we don't need a repeats argument, and Rest can just receive its behavior from the enclosed pattern.

@jamshark70
Copy link
Author

jamshark70 commented Nov 2, 2019

The functionality of partial embedding a pattern seems to be a more generally useful one, independent of Rest. Then we don't need a repeats argument, and Rest can just receive its behavior from the enclosed pattern.

We already have Pfin.

Consider this use case:

  • 5 notes of duration 0.25
  • Followed by one rest
  • Then 5 notes
  • Then one rest, whose duration has increased in an arithmetic series from the last rest
  • and repeat these 2 elements indefinitely.

Currently the user is required to write: Pseq([Pn(0.25, 5), Pfin(1, Pseries(0.25, 0.25, inf).asStream).collect(Rest(_))], inf) (edit: initially I forgot the Rest part... which underscores how over-complicated it is -- I know what I'm doing and I still left part of it out). I'm saying that this is clumsy and forum questions suggest very strongly that users expect this requirement to be very simple to write -- they expect it to be easy to insert a single rest into a sequence -- when in fact it is anything but simple to write.

I would go so far as to say that this is the normal, baseline case for variable-valued rests, and it's one that we support relatively badly.

@telephon
Copy link
Member

telephon commented Nov 3, 2019

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

Consider this use case:

All I am saying that whatever is required from rests is also required from any other value, like a number or an event. I am not arguing against your proposal but try to see it as independent of the Rest class itself.

So consider this use case:

  • 5 rests of duration 0.25
  • Followed by one note
  • Then 5 rests
  • Then one note, whose duration has increased in an arithmetic series from the last note
  • and repeat these 2 elements indefinitely.

Note values, unlike rests, can't carry a duration, so this seems what is skewed in the whole picture.

@jamshark70
Copy link
Author

jamshark70 commented Nov 3, 2019

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

I'm not quite following what you mean by "parallelism" -- if you are partially embedding a stream and you want the stream to resume the next time it comes around (resume, rather than resetting), then that would seem to be fundamentally incompatible with parallelism (i.e., if parallelism is the point, you probably aren't using Pfin with a stream anyway).

All I am saying that whatever is required from rests is also required from any other value, like a number or an event. I am not arguing against your proposal but try to see it as independent of the Rest class itself.

So consider this use case:

Ok, I see. There are two differences:

  • "5 rests of duration d" can be optimized into one rest of duration 5*d, greatly simplifying the pattern's topology. (Counterargument: There could be another element in the Pbind that needs to advance once for each of the 5 rests, in which case you can't optimize.) Also make those 5 rests variable duration and it's more convincing.

  • A note of some variable value is currently easy to write, but wrapping that variable value in something else is not so easy. A better analogy here might be the way we have to wrap arrays in a second array level for arrayed synth arguments. That expands the picture from "Rest-ifying" to a more general wrapping.

    • But I'd maintain that the normal case for a rest is to embed one while the normal case for other wrapping would be to embed as many values as the source pattern produces. Rests are unique in that a sequence of rests is indistinguishable from a single rest summing their durations.

So it would be worth using this as an opportunity to simplify array arguments as well.

@telephon
Copy link
Member

telephon commented Nov 4, 2019

Pfin combined with a stream breaks parallelism, but you can wrap the outer pattern in a Plazy.

I'm not quite following what you mean by "parallelism" -- if you are partially embedding a stream and you want the stream to resume the next time it comes around (resume, rather than resetting), then that would seem to be fundamentally incompatible with parallelism (i.e., if parallelism is the point, you probably aren't using Pfin with a stream anyway).

No, it can be done (example uses no rest for simplicity):

// patterns take two instead of one
(
var halftoneLadder = Pn(Pseries(0, 1, 24)).asStream;
a = Pbind(
	\note, Pseq([0, 1, 2, 3, Pfin(1, halftoneLadder), 3, 2, 1], inf),
	\dur, 0.1
);

Ppar([a, a <> (octave: 6)]).play

)



// both patterns take only one
// (edited, first version was the wrong way round, sorry!)
(
a = 
Plazy({
	var halftoneLadder = Pn(Pseries(0, 1, 24)).asStream;
	{
		Pbind(
			\note, Pseq([0, 1, 2, 3, Pfin(1, halftoneLadder), 3, 2, 1], inf),
			\dur, 0.1
		)
	}
}.value);

Ppar([a, a <> (octave: 6)]).play

)

Currently, using Rest, we can parallelize patterns. The change as you suggest it breaks this assumption. I'd like to have a container for patterns that works like the example above, but is simple to write, and then use that as a blueprint for the partial embedding of rests as well.

What I don't like is the implicit conversion from a stateless to a stateful internal state, with no indication that this is happening from the semantics of Rest (apart from usefulness).

Alternatively, a message like:

iterest { |n|
    Pfin(n, this.iter).collect { |x| Rest(x) }
}

… may do the job?

Your example would then look like this:

// you have to add .asStream
(
p = Pbind(
	\degree, Pn(Pseries(-7, 1, 15), inf),
	\dur, Pseq([Pn(0.125, 5), Pseries(1, 1, 4).iterest * 0.125], inf)
).play;
)

@jamshark70
Copy link
Author

jamshark70 commented Nov 17, 2019

Well, I just lost some text because some of the formulations in this thread (streams in looping pattern structures) are actually serious infloop risks. I'll file a bug report about that. EDIT: supercollider/supercollider#4642

Anyway, to rewrite the main points quickly:

  • Your example does convince me that Rests should remain stateless.

  • I don't see a big gain for iterest -- .iter.rest(n) is only two characters more but considerably more readable (among other reasons, for avoiding the appearance of misspelling "interest").

  • I'm quite concerned about the infloops. We don't see it very often because writing a stream into a pattern is currently a rare use case. If it becomes the way to handle interlaced rests, people will do it more often and that will expose the problem. But that's more general than just Rests.

@telephon
Copy link
Member

telephon commented Nov 17, 2019

* If it becomes _the_ way to handle interlaced rests, people will do it more often and that will expose the problem. But that's more general than just Rests.

Maybe it is possible to interlace in a stateless way. I have a number of old experiments, I'll check which worked.

@telephon
Copy link
Member

I've found a version that allows you to embed a number of items into a parent stream and continue from where you left off. It might be an issue that it holds on old threads and streams (usually Routines), which might be solvable in some way.

Pblock : Pn {

	var streamDict;

	embedInStream { | inval |
		var outval, stream;
		stream = this.getStream;

		repeats.value(inval).do {
			outval = stream.next(inval);
			if(outval.isNil) {
				^inval
			};
			inval = outval.yield;
		};

		^inval

	}

	getStream {
		var stream;
		streamDict = streamDict ?? { IdentityDictionary.new };

		stream = streamDict.at(thisThread);
		if(stream.isNil) {
			stream = pattern.asStream;
			streamDict.put(thisThread, stream);
		};
		^stream
	}



}

+ Pattern {
	block { | n=1 |
		^Pblock(this, n)
	}
}

// a test


(
a = Pblock(Pseq([1, 2, 3], 2), 2);

b = Pseq([a, 30, 40], inf);
c = Pseq([a, 30, 40], inf);


b.asStream.nextN(20).postln;
c.asStream.nextN(20).postln;
"";
)

// returns
[ 1, 2, 30, 40, 3, 1, 30, 40, 2, 3, 30, 40, 30, 40, 30, 40, 30, 40, 30, 40 ]
[ 1, 2, 30, 40, 3, 1, 30, 40, 2, 3, 30, 40, 30, 40, 30, 40, 30, 40, 30, 40 ]

This does not solve supercollider/supercollider#4642.

@jamshark70
Copy link
Author

Hm, that's quite nice.

For garbage collection, perhaps the end condition could remove the entry:

	if(outval.isNil) {
		streamDict.removeAt(thisThread);
		^inval
	};

@telephon
Copy link
Member

Yes, that would be a possibility. But then, the blocked pattern would loop forever. But one could keep a set of those threads whose streams have ended. Of course this would keep a reference to that thread. So instead of the thread itself, the identifyHash could be used, and we'd be free of troubles.

@telephon
Copy link
Member

Pblock : FilterPattern {

	var <>repeats;
	var streamDict, endedStreamThreads;

	*new { | pattern, repeats = 1 |
		^super.newCopyArgs(pattern, repeats).initDict
	}

	initDict {
		streamDict = IdentityDictionary.new;
		endedStreamThreads = IdentitySet.new;
	}

	embedInStream { | inval |
		var outval, hash, stream;

		hash = thisThread.identityHash;
		stream = this.getStream(hash);

		repeats.value(inval).do {
			outval = stream.next(inval);
			if(outval.isNil) {
				this.removeStream(hash);
				^inval
			};
			inval = outval.yield;
		};

		^inval

	}

	getStream { |hash|
		var stream;

		if(endedStreamThreads.includes(hash)) {
			^nil
		};

		stream = streamDict.at(hash);
		if(stream.isNil) {
			stream = pattern.asStream;
			streamDict.put(hash, stream);
		};
		^stream
	}

	removeStream { |hash|
		streamDict.removeAt(hash);
		endedStreamThreads.add(hash);
	}


}


+ Pattern {

	block { | n = 1 |
		^Pblock(this, n)
	}

}

@telephon
Copy link
Member

telephon commented Nov 18, 2019

This may also grow indefinitely, but uses less memory.

EDIT: identityHash is not safe here really, because different objects may have the same hash. It would be really handy to have a way to reduce this risk. One way in this case could be instVarHash – but that is slow.

@joshpar
Copy link
Member

joshpar commented Dec 8, 2019

@jamshark70 - at this point, what amount of discussion is still needed? Or do we feel like we can decide where all of this should move forward?

@joshpar
Copy link
Member

joshpar commented Dec 8, 2019

(In other words - can we finalize what should be done at this point to move this to a new feature?)

@jamshark70
Copy link
Author

Thanks for the reminder. I'm just going into a day and a half of workshops, bit busy.

In short, I think we should go with the simpler alternatives for pretty much all the aspects. I think asRest instead of .rest (to avoid rest vs Rest confusion). If users want a persistent stream, they should write it explicitly. This may annoy some users but a/ Julian is correct that naively auto-streaming breaks parallel usage and b/ while his wrapper class is a clever idea, the persistent references will never be deleted because we don't have a stream destructor, and I don't think we should put something into core that we know will prevent temporary streams from being garbage collected. (Realistically, we're not going to add a stream destructor and users wouldn't use it consistently anyway.)

So it would look like Prest(Pseries(...).asStream) or Pseries(...).asStream.asRest.

I think it wouldn't hurt to have a .value call in Rest's value method, to enable a usage like Rest({ rrand(1, 3) }). I can't check right now whether that's already there or not.

@joshpar
Copy link
Member

joshpar commented Mar 2, 2020

@jamshark70 - do you feel like this is now at a stage for consideration to work on for 3.12?

@jamshark70
Copy link
Author

do you feel like this is now at a stage for consideration to work on for 3.12?

I think there is some overall consensus on what the feature should look like. For a handful of reasons, including health reasons, I can't commit to write the PR myself at this time. I'd be delighted if other interested parties took it over. Otherwise, I guess we should close this one.

Thanks for checking in.

@telephon
Copy link
Member

telephon commented Mar 4, 2020

Unfortunately I can't follow up on this currently either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants