Harmonized expand, expandAs and repeatTensor

[nicholas-leonard]

Like the view method, the subject methods now accept an optional result tensor. The PR includes two new unit tests : expand and repeatTensor.

Also fixes #59.

[soumith]

what are these introduced for? They don't seem to be necessary

[soumith]

Could you also rebase to a single commit, while git creates a merge commit that we can quickly revert, I was looking back at the commit history, and it was very hard to follow commits and see if they were tied to a particular PR. Two or 3 commits in a PR is justified, but considering that this is a small change, I would want this in a single commit.

[nicholas-leonard]

@soumith @koraykv @clementfarabet @andresy As suggested by @soumith I am opening the new policy concering less commits per PR for debate.

Personally, I like to have more commits as this is part of my work process. I have a commit for my initial version of a new class/function/feature, another for the unit test, another for the final debugged process, and so forth and so on. I also like having more commits as it increases my commit count, which looks nice in a CV, the open-source report cart, and the github profile.

I also I don't like rebases, they change the dates of commits. I don't think we should tell people how many commits they can do, especially after those commits have already been made. I know many github users try to maintain commit streaks (like 138 days of at least one commit per day). Asking for a rebase can reset a streak if the user expects it to pass (not my case for this PR, but could easily have been).

I also think its a bad idea to have one commit over too many files and changes. The commits should capture the development process. As long as they are well documented (a pertinent -m message), they should be accepted. I think its great that we are enforcing indentations, unit tests and documentation. But enforcing a number of commits per PR seems like micro-management. It could dissuade users from joining Torch.

As for reverting PRs, this shouldn't happen as often as it has. The code should be reviewed and the entire build should be unit tested before a PR is accepted. I have sometimes seen a travis build fail after a PR was merged. Yes we as users are responsible to test our own changes before submitting the PR, but sometimes we can forget (I once tested without realizing that I forgot to re-luarocks make my changes). Anyway, big rant for a small itch.

[soumith]

contrary to what github says, this wasn't merged.

[soumith]

@nicholas-leonard we've decided that in the long term, rebasing commits to logical blocks makes more sense. Or main criterion is always keeping torch maintainable and clean, and optimizing contributor github profiles is not a factor for us. While this has not been a strict requirement at all in the past, we've decided that the time is right to make this a hard-practice.

We added a CONTRIBUTING.md that formalizes this process, and we would like all features to be rebased to a single logical block. It is pretty simple to do with git's interactive rebase ("git rebase -i").

The changes you made to Tensor.lua also need to be removed. They are unnecessary in our perspective . If you would like to use tensor.isTensor, please do so in user-space.

Thanks for your contributions.

[andresy]

We are not running after Github streaks. Or if we do, we want good streaks, which maximize rational additions with respect to each commit. Otherwise I would just run a stupid cron. As torch grows, we have to be sure the history does not become unreadable. If sincerely feel sad when I hear about these borderline practices.

[nicholas-leonard]

I am so disappointed... I love torch, but this was a cheap move on your part. You could have at least included me in the debate. On the other had, you know I will continue to contribute anyway, because torch is awesome. Nicely played.

[nicholas-leonard]

@andresy Nice to meet you again (we met at NIPS last year). Anyway, I agree with your streaks argument, but I still think enforcing a strict one commit per PR policy is a form of micro-management, and another barrier of entry into torch. On the other hand, yes, I admit, it keeps the commits clean. But now I have to learn git rebase, and I am so lazy.

[andresy]

Ah, sorry... I am afraid I am very dictatorial in terms of managing torch, and even though I have softened my dictature in the last years, I still like to keep it my way :) I have been thinking of adding rules since a long time, but only recently we had to look at that into details. Probably more rules will come soon...

[jonathantompson]

If the torch git history was so important to the main contributors, how did they justify the decision to move to a new repo (from torch7-distro)?

On Sep 22, 2014, at 8:16 PM, Ronan Collobert notifications@github.com wrote:

We are not running after Github streaks. Or if we do, we want good streaks, which maximizes rational additions with respect to each commit. Otherwise I would just run a stupid cron. As torch grows, we have to be sure the history does not become unreadable. If sincerely feel sad when I hear about these borderline practices.

—
Reply to this email directly or view it on GitHub.

[nicholas-leonard]

#68

[soumith]

This thread is derailing publicly and privately, lets just close this topic here.
@nicholas-leonard just fyi your comment is what triggered an internal debate among the maintainers, and it was basically unanimously agreed upon to have properly rebased commits. This is standard practice in large codebases, we were pretty lax about these things in the past, but as the code grows things will become more structured.

@jonathantompson the git history was preserved when splitting from torch7-distro to torch7, and it was a design decision that in retrospect I largely agree with (though I wasn't part of it). Dont be angry still :) I made you a distro repo

[jonathantompson]

"the git history was preserved when splitting from torch7-distro to torch7"

Do you know if there is a way to link the new repo to the old one (this might be wishful thinking)? It would be great if the earlier commits showed up in git log or git blame? Right now it's kinda hard to track down who wrote what without going to torch7-distro.

One completely unrelated question: There has been a bit of work lately getting multi-gpu support into Theano... Do you know if anyone has started looking at this for torch?

[soumith]

@jonathantompson about multi-GPU support, lets move the conversation here torch/cutorch#42

[soumith]

I've added a comment detailing what's involved, it's fairly trivial to implement

[andresy]

@jonathantompson well, splitting the git repo was actually a nightmare, to keep as much as history as possible. Because things had to be moved around to create the standalone packages, I had to create new git subtrees, filtered in the proper way to keep the right history while keeping file history. We looked into this for quite a while, and we did not find a better alternative. To my knowledge, Github has not way to link such new repo to the original repo (which is not surprising, given the efforts and tweaks to do it properly with git itself).