Return API for Scaper.generate

[pseeth]

The sequence of changes discussed in #105 contains in it an option to return something from Scaper.generate. After offline discussion, we realized this would be better served in its own issue, and in a PR that get's merged post #105.

[justinsalamon]

Pulling in latest relevant comment so we can continue here:

Regarding return API, it should be the same regardless of how the options are set.

Focusing on the most common use case, I'd start with:

• soundscape_audio, soundscape_jam

But we also have the text file... I wonder if we should return the list that's used to create it. Then we's return:

• soundscape_audio, soundscape_jam, soundscape_list

Only then would I return isolated events, so now we get:

• soundscape_audio, soundscape_jam, soundscape_list, event_audio_list

thoughts @pseeth ?

[justinsalamon]

Based on offline discussion:

• This return API looks good: soundscape_audio, soundscape_jam, soundscape_list, event_audio_list
• The soundscape_list will be a python list - the one used to created the CSV file right now. @pseeth this means only foreground events will be registered in this objects. Thoughts?
• event_audio_list - will contain only FG events, yes?

This also means we'll probably have up update the call API, which is currently this:

• https://github.com/justinsalamon/scaper/blob/master/scaper/core.py#L1901-L1904

  def generate(self, audio_path, jams_path, allow_repeated_label=True,
               allow_repeated_source=True,reverb=None, save_isolated_events=False, 
               isolated_events_path=None, disable_sox_warnings=True, no_audio=False, 
               txt_path=None, txt_sep='\t', disable_instantiation_warnings=False):

In particular, we'll have to think about audio_path, jams_path and no_audio.

The least disruptive would be to change the first two into option args with defaults=None, same as txt_path. This way if the user provides them everything works the same as before. If the user doesn't, then nothing gets saved to disk. We could, in theory, leave no_audio the same, in which case the function returns None for the soundscape audio and the event audio list.

@pseeth thoughts/comments?

[pseeth]

I like turning everything into an optional arg with =None as the default. Returns look good!

[justinsalamon]

Hmm, on second thought, if we make audio_path optional it suggests that if it's left set to None then the audio should not be generated, at which point no_audio becomes redundant and it'd make sense to remove it, which in turn would break the API.

Options:

1. keep audio_path and jams_path positional (non-optional), and just update what the function returns
2. Make audio_path and jams_path optional and keep the redundant no_audio option such that if audio_path is None then no audio is returned, if it's not None but no_audio is True then still no audio is return. Feels messy to me.
3. Make audio_path and jams_path optional, remove no_audio, and only integrate this change as part of a new, API-breaking release.

It may sound "extreme", but I am actually partial to option 3. You?

[pseeth]

Hmm, I think audio_path=None implies that the audio won't be saved, rather than be generated. Whereas no_audio implies it's not going to be generated at all. So I think they should co-exist. When no_audio is True, then, what should the return API be? Return soundscape_audio = None, and event_audio_list = None?

[justinsalamon]

When no_audio is True, then, what should the return API be? Return soundscape_audio = None, and event_audio_list = None?

Yes, I think when no_audio is True we should return None for all items that are expected to contain audio.

Given this API though, I don't love the term "no_audio", because it's a little vague. But I can live with that and then when we break the API we can consider renaming this too.

OK, I think we have a path forward for this now. I'll work on a PR.

[pseeth]

Awesome, yeah I agree that no_audio should be changed to something else in a future release.

[justinsalamon]

Addressed via #121. Some missing functionality moved over to new issue #122. Closing this one out.