-
Notifications
You must be signed in to change notification settings - Fork 103
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
Ensured the Identify Key checkbox works correctly in Row Names or Numbers dialog #8529
Ensured the Identify Key checkbox works correctly in Row Names or Numbers dialog #8529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom that new first line generates an error. The one:
all(sapply(rownames(survey), is.numeric))
I don't think it is needed, so you can delete it.
I am a bit confused how you managed to test it without finding that error yourself.
Originally I think the extra code was introduced because often the row names are just numeric, and the code finishes with them as character. I am now prepared to live with that, at least for now!
@rdstern apologies, I think i read the issue wrongly. I have made the changes your requested. but you second paragraph is a bit confusing, Please did you want the dialog to have the original four lines again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom thanks, it works now!
I certainly did not want those 2 lines added again That is what made it go wrong last time.
But the reason they were added was that sometimes the row variable is numeric and sometimes it is character. (In the datasets in the library the mtcars data has character rows if you want to see an example.
Now, the result - from tyour code is always a character column. It would be nice if it were nemeric when appropriate and stayed as sharacter otherwise. So I have been checking and I think adding this line is all that is needed.
a) Maybe you could try adding that.
row <- Hmisc::all.is.numeric(row,what=c("vector"))
(Of course the name row is whatever name the variable will be.)
Here it is, in place:
b) Once done, I would like @lilyclements to confirm this is a sensible addition.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekagorhom that now looks fine - thanks. @lloyddewit for the last week I am afraid (at least for now) - over to you!
fixes #8510
The changes are;
a) Delete the second set of lines.
b) To permit the key variable checkbox to be unchecked while still being checked by default.
c) added the function
all(sapply(rownames(data), is.numeric))
@N-thony , @rdstern this PR is ready for review
Thanks