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

Proposal on the way we work #37

Open
keunwoochoi opened this issue May 1, 2019 · 7 comments
Open

Proposal on the way we work #37

keunwoochoi opened this issue May 1, 2019 · 7 comments

Comments

@keunwoochoi
Copy link
Owner

keunwoochoi commented May 1, 2019

Hi all - including other maintainers and everyone,

Thank for all the works, everyone. I'd like to share my feeling and suggestions on the way we work on this repo. For me, there seem two aspects that we can make an improvement.

Doing things quicker

Yes, we didn't reach a unanimous agreement yet on some of the issues. But I'm somehow worried if we're seeking perfection for the sake of being speedy/practical.
At the end of the day, we're working on "-contrib" repo, I think we really should value the speed more than now while feeling ok with some non-perfect decisions. We can simply change it. To be honest we can make a lot of breaking changes. I feel like we're even more cautious than fb/pytorch main developers when pytorch version<1.0.

Hence more open

It's related to the previous point. Maybe, we can lower the bar for PR/merging (e.g., sensible names, arguments, and unit test?), and refine them after merging, right? Because I'm afraid the current process seems, as it takes long to get merged, naturally sort of discouraging; when someone considers putting some effort on it, chances are they conclude that their time/effort doesn't get returned -- I had some feedbacks on this and I think it makes sense.

Overall, because it's -contrib repo, I'd like to suggest something like this:

  • anyone can suggest some new function
  • they make PR
  • if there's no error and it's relevant, we merge it
  • out of the merged ones (=the master branch at any time), we then do a deeper review. Once we're sure that it's good to go (=the seemingly current criteria we're applying to when we're reviewing), we (or they or anyone) make a PR to torch-audio main repo.

This is nice since there are multiple milestones, the participants get rewards more often by making gradual progress.

I'd definitely like to hear from other maintainers @faroit @f0k @ksanjeevan as well as other participants. Please share your thought!

@soumith
Copy link

soumith commented May 1, 2019

this is generally a great direction when doing fast iteration and collecting functionality rather than deeply thinking about designing a coherent package.
It's a perfect model for -contrib who's primary purpose should be to collect functionality while thinking about final design.

@ksanjeevan
Copy link
Collaborator

Yeah I agree. A lot of the discussions in the issues have been very good and brought us to definitive conclusions but others just by their nature will take longer to settle. I think it's better if we press forward with the things we know can improve the project incrementally, allowing for overarching discussions to not be rushed.

@keunwoochoi
Copy link
Owner Author

(wanna copy even more people - @densuh @hagenw @drscotthawley @nkundiushuti @VinodS7 @dhpollack @hbredin)

@keunwoochoi
Copy link
Owner Author

Actually, this also changed my mind a bit - once we're done with STFT, I'd like to add harmonic-percussive source separation #25 which seems a good example of {Yes to be in -contrib repo, maybe not in the main repo, people can still use it}. There are a bunch of these kinda things. Maybe the same for preemphasis function as another example?

@keunwoochoi
Copy link
Owner Author

Workflow suggestion

  • (Optional) Maybe discuss what to add at Issue.
  • Make a PR withLayer and Functional implementations (+ unit test, preferably)
  • If it works, we merge it here
  • We then keep optimizing the implementation, parameter, names, default values, etc.
    • So far people should it at own risk
  • After optimization, we make a PR to the main repo. By 'we' it could be the original author, or any of the maintainer (let's not bother this too much)
    • This is when we feel more responsible for it, along with the main torchaudio repo maintainers.

Action items

  • Finish Spectrograms etc as a good example
    • Add unit test for it, which will also serve as a test template

@keunwoochoi
Copy link
Owner Author

Just for the record, I think at the beginning we were more like strict partly because when @faroit and I were brainstorming it, we saw this repo more like a replacement of torch/audio. Only at the very moment of making it public, we named it -contrib. But overall I think we're good to go in this way (I also talked over email with @f0k). Let's do it!

@drscotthawley
Copy link
Contributor

This seems like a welcome change.

Somewhat related: this is probably old news to you, but I'll post it anyway b/c I missed it: Brian McFee's group put out a paper last winter on Open-Source Practices for Music Signal Processing Research. Justin Salamon posted a PDF of the preprint on his website.
Although this paper is more about the technical setup rather than on "how we work."

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

No branches or pull requests

4 participants