Arbor support

[anilbey]

Reopen of #393 with restored git history.

[lukasgd]

Hi @anilbey, who asked you to open a PR for Arbor? You are essentially taking over a finished contribution (#393) that we've developed over the last 6 months, committed a few stylistic changes and small tests (but no new functionality) without announcing it even after we agreed with the maintainers on merging the PR. I carefully factored your ideas into the new contribution, avoided regressions and made it consistent with the rest of BluePyOpt, but then you decide to close the PR to reopen it here? I do not approve of this and the code being used here - please close this PR.

[wvangeit]

Dear @lukasgd , this new PR was the result of @anilbey losing his history in the original PR. As I mentioned in the original PR please create a new one with the history restored to last week with @anilbey 's latest changes. Then we can close this PR and work further on yours using code review.

[lukasgd]

Hi @anilbey, who asked you to open a PR for Arbor?

Where do you answer this question, @wvangeit? Are you aware that @anilbey has pushed his code to my fork without any prior discussion with anyone from the Arbor team? We had a call on Oct 11 in presence of @DrTaDa, @thorstenhater and @brenthuisman, where you orally approved merging the PR after I would do some final changes. You know about my personal situation and the associated time constraints. Raising a few more minor requests such as @anilbey did in the following with improved docs is fine and I've addressed them.

But pushing major changes that break the structure and correctness of the new contribution without adding features and cluttering up the commit history with very many tiny commits for essentially a few unfinished unit tests (of previosuly end-to-end tested code) without asking is not ok. Adding useful tests is welcome of course and so is tiny fixes like you did them in the past, but for the rest there is code review and for major changes he could have opened a PR to my fork.

Nevertheless, I've gone ahead and fixed these tests, added more comprehensive end-to-end ones and also completed the half-finished function renaming/-grouping he started. I've cited Anil correspondingly in 73772ea. It's not correct to say that he lost his history. It's preserved in arbor_integration_anil_review and referenced in the PR and the reasons are given in the accompanying comment in #393.

If you're interested in BluePyOpt as a product, I would expect you to be very satisfied now with the state of the code in the branch of #393 (the complaint on the failing test is outdated - it was one for Neuron btw and seems to point to a problem in the master branch). But it seems you and @anilbey are probably interested in something else. I'm afraid I don't have time to dig out the useful parts of these numerous commits just for the sake of a few unit tests.

Closing #393 and reopening it here is essentially hijacking it. This is a scientific no-go. If I see this one still open mid next week I will intervene at EPFL.

We won't collaborate on this outside of the original PR. If you want to continue the discussion, continue it there.

[brenthuisman]

Hi @wvangeit @anilbey, you took us a bit by surprise with this. I think it would have helped if you had communicated this in one of our meetings; we work with the squash-and-merge strategy, which is also quite common, and apparently we were insufficiently aware you manage your PRs differently. This does not need to be a problem, we could have found a way to revert the history to suit your requirements, and I hope that we still can.

This feature is @lukasgd's final project for us (at least, for the time being, we would not mind if he circles back after his medical internships are over 🙂), so I hope you can understand why this sudden change was upsetting.

I propose that we set up a meeting to sort this out, which I have no doubt that we can. Since I work part time and @lukasgd is spending his working hours on other matters, I'll take maybe a few days to send around an invitation/doodle, but ASAP.

[wvangeit]

@brenthuisman Yes, I guess it's a bit of communication issue. I did say in our last meeting that the PR looked ok for me, but I also mentioned that I would give @anilbey, who unfortunately couldn't join that meeting because he had other obligations, a last chance to have a look at it.
@lukasgd, here I mentioned to create a new PR with the history restored: #393 (comment)
We are aware, @lukasgd, of you're limited availability, that's also why it made sense that @anilbey made some changes himself in your branch (which you gave us the ability to do) so that you wouldn't have to do it. If you didn't like that you could have disabled that ability from the start, and we could have done the code review using comments.
Our only point that we insist upon is the restoration of the history. You squashed some commits of @anilbey with your own, basically claiming authorship on some of these changes, which 'we' consider 'a scientific no-go'. So we just want to have that restored.
We know that's common to squash a PR before merging, but we're not at that stage yet, and I mostly try to avoid that so that we can see exactly who made which changes.
The new PR was not at all meant to claim anything from you, all the commits are still there, under your name. It was just the only way for @anilbey to restore the history from his side. Since you object to the fact the PR itself was created by @anilbey , which in my opinion was rather a technicality, I'll close that one. But we still need a new one with the old history.

[lukasgd]

We are aware, @lukasgd, of you're limited availability, that's also why it made sense that @anilbey made some changes himself in your branch (which you gave us the ability to do) so that you wouldn't have to do it. If you didn't like that you could have disabled that ability from the start, and we could have done the code review using comments.

I think I've answered that already in #422 (comment). The permission to make changes is there to enable collaboration, but not for making large, unannounced changes not in line with previous development (there is code review to discuss such ideas).

Our only point that we insist upon is the restoration of the history. You squashed some commits of @anilbey with your own, basically claiming authorship on some of these changes, which 'we' consider 'a scientific no-go'. So we just want to have that restored.

I think this is a bit of an apples and oranges comparison. The code of @anilbey I've integrated is essentially a few unit tests and an idea how to regroup functions - a rather small part of the PR. Both of these things are useful, but not finished, so I fixed and extended them (as commented in #393 (comment)). To avoid any of the regressions and come to results in manageable time, I've built on the previously known-to-work state. I have no incentive to "claim authorship" on these ideas and correspondingly cited Anil in the commit message.

As mentioned before, @anilbey split his work into many commits, moving large chunks of code around in them, and the number of possible diffs just becomes unmanageably high. I had no good way to pick the useful parts of his contribution (or even "squash" commits) when I need to do this in my spare time. This style creates a big maintenance burden for me and I think you should factor this in when you are collaborating with external people whose support you depend on. I understand that during development it can be useful to commit changes often, but the resulting Git history is not the most effective way to share your work.

I don't have a problem with @anilbey being the Git author of the mentioned parts. If you want to see his commits show up in the history, but are ok with the end result in arbor_integration, we can make it happen in #393.

The new PR was not at all meant to claim anything from you, all the commits are still there, under your name. It was just the only way for @anilbey to restore the history from his side. Since you object to the fact the PR itself was created by @anilbey , which in my opinion was rather a technicality, I'll close that one. But we still need a new one with the old history.

Thank you for closing this PR. I still don't quite understand why it was necessary when @anilbey's history was already preserved and referenced under arbor_integration_anil_review. We could have just had this discussion under the original PR. I wouldn't call that one a technicality btw as it's pretty much a progress journal where everyone involved contributed their ideas in order and it's linked by several open issues in Arbor and BluePyOpt itself. Some things are still nowhere documented and I'd plan to attach them there. So, I'd ask you to reopen that one, then we can discuss how to change the recent Git history so that you are satisfied.