Return API for Scaper.generate
pseeth opened this issue · 8 comments
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 ?
Based on offline discussion:
- This return API looks good:
soundscape_audio, soundscape_jam, soundscape_list, event_audio_list - The
soundscape_listwill be a pythonlist- 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:
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?
I like turning everything into an optional arg with =None as the default. Returns look good!
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:
- keep audio_path and jams_path positional (non-optional), and just update what the function returns
- Make
audio_pathandjams_pathoptional and keep the redundant no_audio option such that ifaudio_pathis None then no audio is returned, if it's not None butno_audiois True then still no audio is return. Feels messy to me. - Make
audio_pathandjams_pathoptional, removeno_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?
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?
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.
Awesome, yeah I agree that no_audio should be changed to something else in a future release.