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

Add conversion & unit tests for dgR, dtR & dsRMatrix #139

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
4 participants
@binxiangni
Contributor

binxiangni commented Jun 19, 2017

In order to solve #17 and #114. And there is a piece of codes using the algorithm from scipy. I have added a reference to that. What else should I do to avoid violating the copy rights? @coatless @eddelbuettel @thirdwing

Btw, I squash and merge a branch called Rsparse to the master, but now the commit history still shows up here. I know the reason might be that I didn't squash and merge the previous commits, which leads to a messy timeline. :(

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 19, 2017

Member

I think you are doing something wrong.

You have not pulled or merged or ... the master branch. Most of these commits here we already accounted for.

That is not in and by itself a big problem, it's just that we could do this more elegantly.

Member

eddelbuettel commented Jun 19, 2017

I think you are doing something wrong.

You have not pulled or merged or ... the master branch. Most of these commits here we already accounted for.

That is not in and by itself a big problem, it's just that we could do this more elegantly.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 19, 2017

Member

It is worth trying one or two things with git. You can rename your current RcppArmadillo working directory and copy of the repo -- say, RcppArmadillo-previous (or add the date, or ...).

The create a fresh checkout. That corresponds to our master branch here.

Then try to get both aligned. You should be able to the the 'working' copy (ie 'previous' above) to just about the same set of commits -- differing only by your new commits made since I merged the branch.

Member

eddelbuettel commented Jun 19, 2017

It is worth trying one or two things with git. You can rename your current RcppArmadillo working directory and copy of the repo -- say, RcppArmadillo-previous (or add the date, or ...).

The create a fresh checkout. That corresponds to our master branch here.

Then try to get both aligned. You should be able to the the 'working' copy (ie 'previous' above) to just about the same set of commits -- differing only by your new commits made since I merged the branch.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Jun 19, 2017

Contributor

Next time around, run git pull --rebase <remote-name> <branch-name> before starting on a PR. This should bring your branch up to the present base without a "Merge branch 'master' of github.com:user/repo"

See Atlassian's Git Rebasing for more.

Contributor

coatless commented Jun 19, 2017

Next time around, run git pull --rebase <remote-name> <branch-name> before starting on a PR. This should bring your branch up to the present base without a "Merge branch 'master' of github.com:user/repo"

See Atlassian's Git Rebasing for more.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 19, 2017

Member

Why rebase and not just merge? The language about 'new commits' tends to confuse me because I do want the existing commits, not the old changes recommitted as new ones.

I think what I do in such a case is to

  • pull the remote master (ie this repository) to be current
  • branch cleanly ie with a fresh git checkout -b feature/newstuff which then contains all of master

But this is git so there are probably half a dozen ways...

Member

eddelbuettel commented Jun 19, 2017

Why rebase and not just merge? The language about 'new commits' tends to confuse me because I do want the existing commits, not the old changes recommitted as new ones.

I think what I do in such a case is to

  • pull the remote master (ie this repository) to be current
  • branch cleanly ie with a fresh git checkout -b feature/newstuff which then contains all of master

But this is git so there are probably half a dozen ways...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 19, 2017

Member

That looks much better 💯

Member

eddelbuettel commented Jun 19, 2017

That looks much better 💯

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 19, 2017

Member

Looks really good. Two quick questions:

  1. You mentioned

And there is a piece of codes using the algorithm from scipy.

Where is that? Does it need a attribution in the file header (i.e. something like 'Portions taken from file abc.py written by D, E and F and released under copyright G' ?)

  1. So what did you do for the cleaner PR? Just a pull from our master, and push? It did the right thing as all the commits we already accounted
Member

eddelbuettel commented Jun 19, 2017

Looks really good. Two quick questions:

  1. You mentioned

And there is a piece of codes using the algorithm from scipy.

Where is that? Does it need a attribution in the file header (i.e. something like 'Portions taken from file abc.py written by D, E and F and released under copyright G' ?)

  1. So what did you do for the cleaner PR? Just a pull from our master, and push? It did the right thing as all the commits we already accounted
@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 19, 2017

Contributor

You can look at the reference here. If attribution can be added, that would be great. It can be "Portions taken from file csr.h written by SciPy developers and released under Copyright 2017 SciPy developers. "(?) I only find that on their website.

  • git clone binxiangni/RcppArmadillo to local and git reset to the commit without all my previous commits.

  • git pull from RcppCore/RcppArmadillo

  • paste the three modified files to the local repo, git add and git commit

  • git push -f to binxiangni/RcppArmadillo

Contributor

binxiangni commented Jun 19, 2017

You can look at the reference here. If attribution can be added, that would be great. It can be "Portions taken from file csr.h written by SciPy developers and released under Copyright 2017 SciPy developers. "(?) I only find that on their website.

  • git clone binxiangni/RcppArmadillo to local and git reset to the commit without all my previous commits.

  • git pull from RcppCore/RcppArmadillo

  • paste the three modified files to the local repo, git add and git commit

  • git push -f to binxiangni/RcppArmadillo

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 20, 2017

Member
  1. My bad. I missed the reference. Yes. something like that would do. I may be hard to get concise author reference for SciPy.

  2. I see. I didn't account for the git push -f possibility.

All good.

Member

eddelbuettel commented Jun 20, 2017

  1. My bad. I missed the reference. Yes. something like that would do. I may be hard to get concise author reference for SciPy.

  2. I see. I didn't account for the git push -f possibility.

All good.

@eddelbuettel eddelbuettel merged commit 8fe1f63 into RcppCore:master Jun 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 20, 2017

Member

All merged.

@binxiangni can you maybe write a short note at each of e #17 and #114 and close them, if appropriate, now that this is merged?

Member

eddelbuettel commented Jun 20, 2017

All merged.

@binxiangni can you maybe write a short note at each of e #17 and #114 and close them, if appropriate, now that this is merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment