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

Add conversion & unit tests for indMatrix #144

Merged
merged 1 commit into from Jul 9, 2017
Merged

Add conversion & unit tests for indMatrix #144

merged 1 commit into from Jul 9, 2017

Conversation

binxiangni
Copy link
Contributor

@binxiangni binxiangni commented Jul 8, 2017

This is the last numeric sparse matrix left. So far the conversion for the numeric sparse matrix should be done.

@eddelbuettel
Copy link
Member

You did really well with the previous PRs! Just check the one commit I did earlier. In general when you use textConnection() you also need to close the connection. So I switched to the newer text=... argument. No other changes from my end.

Oh, and just one thing on wording: you did not implement "file-based" tests as we do not use files :) You could say text-based, or something. Doesn't really matter.

@binxiangni
Copy link
Contributor Author

binxiangni commented Jul 8, 2017

Just finished switching to text=.... There are some tests for sparse matrix conversion in runit.sparse.R, should I move them to runit.sparseConversion.R so that the new test file can contain all tests for sparse matrix conversion?

@eddelbuettel
Copy link
Member

Thanks for switching to text=.... As for moving tests, I have no strong feelings. If they fit better by their functionality, sure. We could also leave them by their historic. Most important is to have tests, and we now have lots which is excellent.

@binxiangni
Copy link
Contributor Author

Some of them in the previous test file might be the same as the new ones in runit.sparseConversion.R. Maybe just put them aside now and integrate them next time.

@eddelbuettel eddelbuettel merged commit cc04640 into RcppCore:master Jul 9, 2017
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