event_time sampling biased when selected time is adjusted due to soundscape duration

Currently, if the chosen event_time is such that event_time + event_duration > soundscape_duration, then event_time gets set to soundscape_duration - event_duration, since event duration takes priority over the event start time.

This was the case for source time and was fixed in #53.

The fix here would be to apply the same solution implemented by @pseeth in #53, i.e we modify the distribution tuple for eve_time so that it's sampled without bias within the adjust possible range.

A quick look at the code suggests we might be able to reuse ensure_satisfiable_source_time, but then we probably want to rename it to something more generic.

@pseeth want to take a stab at this one?

Tagging @jtcramer who first surfaced this.

Sounds good. Let me make sure I understand the spec. So once event_duration is decided, we should just sample with the same distribution but with the upper bound changed to soundscape_duration - event_duration?

I believe so, but of course we need to handle all possible distributions (e.g. including "choose"). Is this logic not already implemented in ensure_satisfiable_source_time?

Yeah, that logic is taken care of already so it shouuuld be a drop-in replacement with the required tests to make sure it works.

OK, but if we're using the same function we probably want to rename it to something more broadly applicable, I think _ensure_satisfiable_time_tuple could work?

Then we'd update the code here:

scaper/scaper/core.py

Lines 1434 to 1461 in a3ca806

    
           # Make sure the selected event time + event duration are is not greater 
        
           # than the total duration of the soundscape, if it is adjust the event 
        
           # time. This means event duration takes precedence over the event 
        
           # start time. 
        
           if time_stretch is None: 
        
               if event_time + event_duration > self.duration: 
        
                   old_event_time = event_time 
        
                   event_time = self.duration - event_duration 
        
                   if not disable_instantiation_warnings: 
        
                       warnings.warn( 
        
                           '{:s} event time ({:.2f}) is too great given event ' 
        
                           'duration ({:.2f}) and soundscape duration ({:.2f}), ' 
        
                           'changed to {:.2f}.'.format( 
        
                               label, old_event_time, event_duration, 
        
                               self.duration, event_time), 
        
                           ScaperWarning) 
        
           else: 
        
               if event_time + event_duration_stretched > self.duration: 
        
                   old_event_time = event_time 
        
                   event_time = self.duration - event_duration_stretched 
        
                   if not disable_instantiation_warnings: 
        
                       warnings.warn( 
        
                           '{:s} event time ({:.2f}) is too great given ' 
        
                           'stretched event duration ({:.2f}) and soundscape ' 
        
                           'duration ({:.2f}), changed to {:.2f}.'.format( 
        
                               label, old_event_time, event_duration_stretched, 
        
                               self.duration, event_time), 
        
                           ScaperWarning)

Right now the code is split on an IF related to time stretching, we can probably pull that lift out to the top of the code block to avoid the current repetition in code (we need to be careful about what changes between the two blocks though, namely the use of event_duration vs event_duration_stretched.

@pseeth since you implemented the original _ensure_satisfiable_source_time do you want to pick this one up? If you're out of cycles I can do this.

p.s. - this might impact some of our regression data!

per offline discussion with @pseeth I'll address this one.

	# Make sure the selected event time + event duration are is not greater
	# than the total duration of the soundscape, if it is adjust the event
	# time. This means event duration takes precedence over the event
	# start time.
	if time_stretch is None:
	if event_time + event_duration > self.duration:
	old_event_time = event_time
	event_time = self.duration - event_duration
	if not disable_instantiation_warnings:
	warnings.warn(
	'{:s} event time ({:.2f}) is too great given event '
	'duration ({:.2f}) and soundscape duration ({:.2f}), '
	'changed to {:.2f}.'.format(
	label, old_event_time, event_duration,
	self.duration, event_time),
	ScaperWarning)
	else:
	if event_time + event_duration_stretched > self.duration:
	old_event_time = event_time
	event_time = self.duration - event_duration_stretched
	if not disable_instantiation_warnings:
	warnings.warn(
	'{:s} event time ({:.2f}) is too great given '
	'stretched event duration ({:.2f}) and soundscape '
	'duration ({:.2f}), changed to {:.2f}.'.format(
	label, old_event_time, event_duration_stretched,
	self.duration, event_time),
	ScaperWarning)