Addsym

[btracey]

No description provided.

[vladimir-ch]

NewSymmetric ... symmetric matrix ...

[btracey]

Oops. Thanks.

[vladimir-ch]

Add a test for a panic with ErrShape?

[btracey]

Done.

[btracey]

PTAL

[kortschak]

Add a check for matrix equality with a Dense?

[vladimir-ch]

Just curious: why is this version of at() comparing Uplo and blas.Upper, while the other at() does not?

[btracey]

Fixed.

[vladimir-ch]

Move this check to set()? Otherwise bound-checking and non-bound-checking set() behave differently.

[btracey]

Done.

[vladimir-ch]

LGTM

We could start using the rebase/squash workflow that is being discussed and see how it pans out. For example this PR could, or even should, be squash to a nice, single commit "Add Symmetric matrix". The subcommits are just noise.

[btracey]

Agreed. Someone will have to walk me through that procedure (not a git master yet).
Waiting for an okay by @kortschak .

[kortschak]

Add new line between funcs.

[btracey]

Done.

[kortschak]

LGTM

The instructions at the cockroachdb/cockroach CONTRIBUTING page are good:

Once ready to land your change, squash your commits. Where n is the number of commits in your branch, run git rebase -i HEAD~n

I would then check that you have the history you expect (one commit ahead of master). Note that you can checkout apparently lost commits, the last one in this PR is 7e92d51, so if everything goes pear shaped, you can git checkout 7e92d5 and have a detached HEAD that is this state with all the subcommits.

and then when you are happy you haven't broken everything, git checkout master, git merge addsym and git push (implicit origin master).

[vladimir-ch]

I like the instructions in https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

[kortschak]

Yes, that's nice. Highlights that I forgot about rebasing master.

[btracey]

Any advice on error handling? I followed the instructions in vladimir's link and replaced all of the "pick" with "squash"

brendan:~/Documents/mygo/src/github.com/gonum/matrix/mat64$ git merge-base addsym master
aef57554f07f87d740f245baffb697baf8cfafa0
brendan:~/Documents/mygo/src/github.com/gonum/matrix/mat64$ git rebase --interactive aef57554f07f87d740f245baffb697baf8cfafa0
error: could not apply ba43817... Added symmetric At, Set, and added tests

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Could not apply ba4381705ce7b2bda57e6e652f8647d66b3c9884... Added symmetric At, Set, and added tests

[vladimir-ch]

I am not sure what exactly you did, so just a couple of questions:

• I see you merged master into your addsym branch, that is unnecessary, or may even be the cause of the problem.
• By "all of the pick" you mean "all except the first one pick", don't you?

[btracey]

• I had the problem before I merged from master. I thought not being up to date with master was the source of the problem. Apparently merging did not fix.
• Yes, I mean all except the first.

[kortschak]

I have a fixed addsym branch I can push to master if you want.

It was a bit messy - repeated remerges from master can cause conflicts.

[vladimir-ch]

I was just about to write that those problems were probably conflicts.

[btracey]

Yea, I saw that there were merge conflicts. Thanks. Working on making sure I actually have one commit now.

[kortschak]

Please update the commit message when you are ready at the moment the commit that would be merged has "Uncommented valid tests and commented invalid tests", which is probably not what we want. There just do a git commit --amend before you push.

[btracey]

Thanks for the help. I followed @kortschak comment and merged locally and pushed to master, though I probably should have followed @vladimir-ch 's and pushed to here. It's merged now.

Thanks for the --amend reminder.

[kortschak]

It looks like we lose the link from the history to the PR.

[vladimir-ch]

Something went wrong, I guess. But this is git, everything is fixable.

[vladimir-ch]

This is what I would have done to merge this pr:

$ git pull
$ git checkout addsym
$ git merge-base addsym master
$ git rebase -i ${HASH}
(replace pick with squash from the second line on)
$ vim file_with_conflicts.go
$ git add file_with_conflicts.go
$ git rebase --continue
$ git rebase master
$ git push -f origin addsym
$ git checkout master
$ git merge addsym
$ git push
(delete addsym branch from github web page)
$ git fetch -p (to purge deleted remote branches)
$ git branch -d addsym

I hope I have made no mistake.

[vladimir-ch]

And next time to avoid squashing, I would just amend the last commit and push -f it:

$ git checkout addsym
(make edits)
$ git add changed_file.go
$ git commit --amend
$ git push -f origin addsym (for a new review)

And then for merging:

$ git pull
$ git checkout addsym
$ git rebase master (in case master has advanced. Fix conflicts using --continue as above)
$ git push -f origin addsym
$ git checkout master
$ git merge addsym
$ git push
(delete addsym branch from github web page)
$ git fetch -p (to purge deleted remote branches)
$ git branch -d addsym

[vladimir-ch]

With the above approach github automatically detects that addsym has been merged into master and closes the branch automatically. @btracey , you had to close the branch manually, didn't you?

[btracey]

Yes I did.

[vladimir-ch]

And that is why this PR is not linked to the merged commit @kortschak . I am not sure, but maybe the key step is to push -f the squashed and rebased branch to github before pushing the master branch. push.default set to simple is very useful here.