Skip to content
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

Armadillo 8.100, call .sync() before accessing the internal arrays of the SpMat class #171

Closed
wants to merge 1 commit into from

Conversation

binxiangni
Copy link
Contributor

@binxiangni binxiangni commented Aug 31, 2017

I just call .sync() in as<>() and it works. But it seems a little weird that no error comes out when I don't call .sync() in wrap() before reading those internal arrays of the SpMat class.

@eddelbuettel
Copy link
Member

eddelbuettel commented Aug 31, 2017

Your tests may be missing the salient fact that we are not on Armadillo 0.8[01]00.* yet.

But it is good that you are proactive and on top of this.

@binxiangni
Copy link
Contributor Author

binxiangni commented Aug 31, 2017

So in addition to copying armadillo & armadillo_bits to here and modifying as<>(), what else should I do?

@binxiangni
Copy link
Contributor Author

I find that I don't need to call .sync() before arma::access::rwp, but I should call that before arma::access::rw, which possibly means no need to call .sync() before directly reading or writing the pointers. I guess that might explain my problem.

@eddelbuettel
Copy link
Member

So in addition to copying armadillo & armadillo_bits to here and modifying as<>(), what else should I do?

I think you can wait for more tests until I had a chance to update Armadillo within RcppArmadillo.

@eddelbuettel
Copy link
Member

eddelbuettel commented Oct 2, 2017

I am now (at last) at Armadillo 8.100.1. I think I will need the first part of the PR -- as there are some errors -- but there are "issues" with this PR as it covers 80+ files it should not cover (from Armadillo 8.*). Shall we / can we clean that up, or shall I just re-apply the relevant part by hand?

@eddelbuettel
Copy link
Member

(Copying over. Wrote this in #175 by mistake at first.)

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.

eddelbuettel added a commit that referenced this pull request Oct 2, 2017
@eddelbuettel
Copy link
Member

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

@binxiangni
Copy link
Contributor Author

Sorry for being late to take action. Being busy with midterms. Kind of difficult to catch you up. So what should I do? As I see, you've removed the redundant files, right?

@eddelbuettel
Copy link
Member

eddelbuettel commented Oct 2, 2017

No real action item. But if you and @sgsokol can look over the changes I made to RcppArmadilloAs.h in the branch that is behind the #176 PR. It is now running a rev.dep -- we should be good.

@binxiangni
Copy link
Contributor Author

I checked it and my part should be fine. Thanks! (btw, the PR number above should be #176 )

@eddelbuettel
Copy link
Member

Ooops. Correcting.

@sgsokol
Copy link
Contributor

sgsokol commented Oct 2, 2017

For me too, it looks OK.

@eddelbuettel
Copy link
Member

Again, sorry to both of for sitting on the PRs in a way that then prevented me from merging them.

Lesson learned, I just should not wait for my next RcppArmadillo release cycle.

@eddelbuettel
Copy link
Member

The content of this PR is now in master via #176. Again my apologies for not merging this sooner (maybe into a branch with arma 8.*).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants