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

Fix documentation of as.matrix to reflect current behaviour #2935

Closed
wants to merge 4 commits into from

Conversation

HughParsonage
Copy link
Member

Closes #2930 from the documentation aspect. There may be a case for nrow(x) == 1 prioritizing column indices, however I chose a documentation change rather than a code change as (a) it's already released, (b) it's consistent with base R.

@HughParsonage HughParsonage changed the title Fix documentation to reflect current behaviour Fix documentation of as.matrix to reflect current behaviour Jun 14, 2018
Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that change, because it is consistent with base R. But I don't like R behavior here, we could eventually address this edge case later but for now doc update is good.

man/as.matrix.Rd Outdated
row names of the returned \code{matrix}.}
\item{rownames}{optional, one of the following specifications of the row names:
\enumerate{
\item If \code{length(rownames) == length(nrow(x))} then \code{rownames} itself
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure but it might not be possible to use \item inside \item{}{... \enumerate{.}}. Probably best move to lower more descriptive sections.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be definitely be nested. I separated them so that I could emphasize the first point later. Too OTT?

Copy link
Member

@jangorecki jangorecki Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so maybe incorrect number/placement of { somewhere. To debug locally use R CMD check data.table_1.11.5.tar.gz --no-tests so you should be able to nail it down much faster.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks!

@jangorecki jangorecki added this to the 1.11.6 milestone Jun 14, 2018
@malcook
Copy link

malcook commented Jun 15, 2018

Out of continued curiosity, what aspect of base R is this consistent with? Thanks

@jangorecki
Copy link
Member

@malcook data.frame instead of data.table

@sritchie73
Copy link
Contributor

Just looking at this now, I actually agree with @malcook and hadn't considered the edge case where the data.table had only 1 row.

@sritchie73
Copy link
Contributor

#2938 now fixes this from a functionality point of view, i.e. making the functionality work the same when the data.table has only 1 row.

@HughParsonage
Copy link
Member Author

@sritchie73 for the record, would you mind rejecting this pull request seeing as yours supersedes?

@sritchie73
Copy link
Contributor

Sure, didn't realise I had the power to do so!

Closing this PR as it is superseded by PR #2938

@sritchie73 sritchie73 closed this Jun 17, 2018
@HughParsonage HughParsonage deleted the as-matrix-doc branch June 26, 2018 12:43
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.

4 participants