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

conversion of simple_triplet_matrix #192

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@sgsokol
Contributor

sgsokol commented Nov 30, 2017

A small patch adding conversion of simple_triplet_matrix format from package slam.
Sorry for all these white spaces removing, it's my new editor Atom who's responsible.
If it's too annoying for you, I'll try to disable this feature and submit with only pertinent part of
modifications.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Nov 30, 2017

Contributor

I'm a bit worried about extending to meet all custom objects defined. Matrix is one thing as it part of the recommended packages. Not so with slam.

Also, please modify this commit to respect spacing.

Contributor

coatless commented Nov 30, 2017

I'm a bit worried about extending to meet all custom objects defined. Matrix is one thing as it part of the recommended packages. Not so with slam.

Also, please modify this commit to respect spacing.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 30, 2017

Member

I think we managed to use slam, SparseM, and SciPy as optional pieces in the tests. That way they do not bubble up into Depends. I would much prefer to keep it that way if we could.

Member

eddelbuettel commented Nov 30, 2017

I think we managed to use slam, SparseM, and SciPy as optional pieces in the tests. That way they do not bubble up into Depends. I would much prefer to keep it that way if we could.

@sgsokol

This comment has been minimized.

Show comment
Hide comment
@sgsokol

sgsokol Dec 1, 2017

Contributor

@coatless

  • I don't think that taking into account slam format adds significant charge on the package. The code is small and compared to previous executions only one type check is added;
  • you are right that Matrix is in recommended packages and not slam, but Matrix is much slower to load than slam. So if someone manages to get all he wants from a small package, why to bother him to force to drag a big one?
  • white spaces are respected now.
    @eddelbuettel I didn't find any mention of slam in unit tests (a part in comments) so I've just put the test in if (suppressMessage(require(slam))) {} block.
Contributor

sgsokol commented Dec 1, 2017

@coatless

  • I don't think that taking into account slam format adds significant charge on the package. The code is small and compared to previous executions only one type check is added;
  • you are right that Matrix is in recommended packages and not slam, but Matrix is much slower to load than slam. So if someone manages to get all he wants from a small package, why to bother him to force to drag a big one?
  • white spaces are respected now.
    @eddelbuettel I didn't find any mention of slam in unit tests (a part in comments) so I've just put the test in if (suppressMessage(require(slam))) {} block.
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Dec 1, 2017

Member

Correct. They way you have the test conditioned we should only need a Suggests:. And on the C++ side it just adds another type. I see no harm in that.

But as a general rule (to both (!!) of you): I generally still prefer issue ticket discussion so that everybody can pipe in before filing PRs. But you two of course have built up some trust by now, but I remain a very mean maintainer (even if I just merged @coatless 's PR to Rcpp....)

Member

eddelbuettel commented Dec 1, 2017

Correct. They way you have the test conditioned we should only need a Suggests:. And on the C++ side it just adds another type. I see no harm in that.

But as a general rule (to both (!!) of you): I generally still prefer issue ticket discussion so that everybody can pipe in before filing PRs. But you two of course have built up some trust by now, but I remain a very mean maintainer (even if I just merged @coatless 's PR to Rcpp....)

@eddelbuettel eddelbuettel merged commit 5d01076 into RcppCore:master Dec 1, 2017

1 check passed

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

eddelbuettel added a commit that referenced this pull request Dec 1, 2017

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Dec 1, 2017

Contributor

@eddelbuettel in my defense, I didn't think you would appreciate an issue ticket for a six character change on a single line followed immediately by a PR ticket.

Contributor

coatless commented Dec 1, 2017

@eddelbuettel in my defense, I didn't think you would appreciate an issue ticket for a six character change on a single line followed immediately by a PR ticket.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Dec 1, 2017

Member

To make it plain, we half a dozen communication channels between us. Pick one. Any one.

Randomly submitting PRs is always the worse alternative.

Member

eddelbuettel commented Dec 1, 2017

To make it plain, we half a dozen communication channels between us. Pick one. Any one.

Randomly submitting PRs is always the worse alternative.

@sgsokol

This comment has been minimized.

Show comment
Hide comment
@sgsokol

sgsokol Dec 4, 2017

Contributor

I realize that even with this patch we still need to load Matrix package to get it working. This invalidates my comment about time differences in loading Matrix and slam :(

Contributor

sgsokol commented Dec 4, 2017

I realize that even with this patch we still need to load Matrix package to get it working. This invalidates my comment about time differences in loading Matrix and slam :(

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Dec 4, 2017

Member

Oh well. It'll be in 0.8.300.1.0 which I am currently rev.dep testing. Some fixes by Conrad in 8.300.1.

Member

eddelbuettel commented Dec 4, 2017

Oh well. It'll be in 0.8.300.1.0 which I am currently rev.dep testing. Some fixes by Conrad in 8.300.1.

@sgsokol sgsokol referenced this pull request Dec 5, 2017

Closed

mandatory Matrix package #194

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