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

Finixh #58 by adding the vardim keyword argument to Tables.matrix, al… #66

Merged
merged 3 commits into from
Mar 11, 2019

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Feb 8, 2019

…lowing the user to specify whether input columns should be materialized as matrix columns or rows

@quinnj quinnj mentioned this pull request Feb 8, 2019
@nalimilan
Copy link
Member

Thanks, looks good! My only hesitation is whether we should call the keyword argument vardim. The only place where that term is used currently AFAIK is in StatsBase's weighted cov, but it's going to be changed to dims for consistency with Statistics.cov. That said, vardim sounds clearer (dims would look weird here) and it could be used for JuliaStats/Clustering.jl#79.

@andreasnoack
Copy link
Member

I think dims and vardim are equally confusing but since the reductions in Base use dims, we have been switching from vardim to dims in StatsBase (although the switch is not yet complete) so I think it would better to use dims because of the precedence.

@quinnj
Copy link
Member Author

quinnj commented Feb 9, 2019

Updated from vardim to dims.

@nalimilan
Copy link
Member

That naming issue is quite annoying. After thinking more about it, I feel like using dims=1 to mean "transform columns to rows" is quite arbitrary. We could as well say dims=2 means "transform rows to columns", or any other intepretation. There's actually no clear relationship with reductions which use the dims argument in Base.

Maybe transpose::Bool=false would be clearer? CSV.file already supports that argument.

@quinnj
Copy link
Member Author

quinnj commented Feb 22, 2019

So what should we do here? I don't have strong opinions.

@nalimilan
Copy link
Member

I think I prefer transpose.

@tlienart
Copy link

I think transpose makes sense here. at the end of the day if this is well documented, it should be fine.

quinnj added 3 commits March 11, 2019 13:23
…lowing the user to specify whether input columns should be materialized as matrix columns or rows
@quinnj quinnj merged commit 1fa8f1a into master Mar 11, 2019
@quinnj quinnj deleted the jq/matrix branch March 11, 2019 19:32
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