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

use native Armadillo batch constructor for dgt, dtt and dst type conversion (clean PR) #175

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sgsokol
Contributor

sgsokol commented Sep 20, 2017

Rebase didn't help but a fresh new fork fixed the things.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 20, 2017

Member

Thanks for a cleaner new PR, much appreciated.

Now, @binxiangni made a good point about the code being able to differentiate between different subtype. Shall we maybe re-add-that feature is a subsequent PR once this one is in?

Member

eddelbuettel commented Sep 20, 2017

Thanks for a cleaner new PR, much appreciated.

Now, @binxiangni made a good point about the code being able to differentiate between different subtype. Shall we maybe re-add-that feature is a subsequent PR once this one is in?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Sep 20, 2017

Contributor

A valid but dangerous method to make a clean PR is to git clone the RcppCore/RcppArmadillo to your local machine, commit on it and then git push -f to your forked repo. Then the PR on #174 can be clean if it is not closed. Just be cautious with the repo name in case you overlap your another repo.

Contributor

binxiangni commented Sep 20, 2017

A valid but dangerous method to make a clean PR is to git clone the RcppCore/RcppArmadillo to your local machine, commit on it and then git push -f to your forked repo. Then the PR on #174 can be clean if it is not closed. Just be cautious with the repo name in case you overlap your another repo.

@sgsokol

This comment has been minimized.

Show comment
Hide comment
@sgsokol

sgsokol Sep 20, 2017

Contributor

@eddelbuettel a commit 4e3ad83 does this differentiation. I have committed it to my master branch (as instructed by github) but I don't know if it will become a part of this PR or of the next one?

Contributor

sgsokol commented Sep 20, 2017

@eddelbuettel a commit 4e3ad83 does this differentiation. I have committed it to my master branch (as instructed by github) but I don't know if it will become a part of this PR or of the next one?

@sgsokol

This comment has been minimized.

Show comment
Hide comment
@sgsokol

sgsokol Sep 20, 2017

Contributor

@binxiangni I have removed my old fork on github, made a new fork and then cloned it locally. It is a little bit radical way but hopefully, it will work without messing things up.

Contributor

sgsokol commented Sep 20, 2017

@binxiangni I have removed my old fork on github, made a new fork and then cloned it locally. It is a little bit radical way but hopefully, it will work without messing things up.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 20, 2017

Member

but I don't know if it will become a part of this PR or of the next one?

If the commit is in the same branch as the previous commits of the PR, then yes. It just grows as you'd expect. Will run a new Travis and just appear here. I'd say try it ...

Member

eddelbuettel commented Sep 20, 2017

but I don't know if it will become a part of this PR or of the next one?

If the commit is in the same branch as the previous commits of the PR, then yes. It just grows as you'd expect. Will run a new Travis and just appear here. I'd say try it ...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 20, 2017

Member

Actually, it actually is showing two commits: 2015884 and then 4e3ad83

Member

eddelbuettel commented Sep 20, 2017

Actually, it actually is showing two commits: 2015884 and then 4e3ad83

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 2, 2017

Member

This was a very important and timely PR. I had test failures due to SpMat changes, but with your syncing it passes.

Now it so happens that I cannot merge this now, but I will give you full credit in inst/NEWS.Rd. Hope that's ok.

I should have put 8.100.* into a branch earlier. My bad.

Member

eddelbuettel commented Oct 2, 2017

This was a very important and timely PR. I had test failures due to SpMat changes, but with your syncing it passes.

Now it so happens that I cannot merge this now, but I will give you full credit in inst/NEWS.Rd. Hope that's ok.

I should have put 8.100.* into a branch earlier. My bad.

@sgsokol

This comment has been minimized.

Show comment
Hide comment
@sgsokol

sgsokol Oct 2, 2017

Contributor

@eddelbuettel I hope only that it is not my action or non-action that prevents you from an ability to merge. If I can do anything to facilitate the merging this or next time just say it.

Contributor

sgsokol commented Oct 2, 2017

@eddelbuettel I hope only that it is not my action or non-action that prevents you from an ability to merge. If I can do anything to facilitate the merging this or next time just say it.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 2, 2017

Member

My bad -- similar to Binxiang's patch which I sat on for too long, I now merged this "by hand". (I neeed Binxiang's for Armadillo 8.100.1, and with that the file was changed.)

But this too is terrific, and very importamt. I added both to the ChangeLog and will update NEWS in a moment before I commit.

Member

eddelbuettel commented Oct 2, 2017

My bad -- similar to Binxiang's patch which I sat on for too long, I now merged this "by hand". (I neeed Binxiang's for Armadillo 8.100.1, and with that the file was changed.)

But this too is terrific, and very importamt. I added both to the ChangeLog and will update NEWS in a moment before I commit.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 2, 2017

Member

Oh, and the earlier comment should gave gone to #171. Ooops.

Member

eddelbuettel commented Oct 2, 2017

Oh, and the earlier comment should gave gone to #171. Ooops.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 2, 2017

Member

Ok, I folded this into #176 which is now being tested.

Member

eddelbuettel commented Oct 2, 2017

Ok, I folded this into #176 which is now being tested.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 3, 2017

Member

The content of this PR is now in master via #176. Again my apologies for not merging this sooner.

Member

eddelbuettel commented Oct 3, 2017

The content of this PR is now in master via #176. Again my apologies for not merging this sooner.

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