Skip to content

Allow selectcols to have tuples as "indices" argument #992

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 24, 2025
Merged

Allow selectcols to have tuples as "indices" argument #992

merged 5 commits into from
Mar 24, 2025

Conversation

ablaom
Copy link
Member

@ablaom ablaom commented Jan 5, 2025

This PR closes #991. It also fixes an unrelated test that seems to have been broken by recent Julia update. Something about the behaviour of @doc, it seems.

@OkonSamuel Let me know if you had a better suggestion for this PR. Otherwise, please review.

@ablaom ablaom requested a review from OkonSamuel January 5, 2025 23:58
@ablaom
Copy link
Member Author

ablaom commented Mar 24, 2025

@OkonSamuel Any chance you could take a look at this?

@ablaom
Copy link
Member Author

ablaom commented Mar 24, 2025

@MilesCranmer Would you be able to review this?

@MilesCranmer
Copy link
Contributor

Apart from that it seems good to me

@ablaom
Copy link
Member Author

ablaom commented Mar 24, 2025 •
edited
Loading

Thanks @MilesCranmer for the lightning-speed review. Outside reviews are essential for maintaining quality of the software and I appreciate them very much.

I've adopted your good suggestions.

@ablaom ablaom merged commit 0a82ccd into dev Mar 24, 2025
3 checks passed
@ablaom ablaom deleted the selectcols branch March 24, 2025 22:47
@devmotion
Copy link
Contributor

FYI this PR - and hence the 1.8.0 release - are broken: Vararg{<:Integer} must be replaced with Vararg{Integer}:

ERROR: LoadError: Wrapping `Vararg` directly in UnionAll is deprecated (wrap the tuple instead).
You may need to write `f(x::Vararg{T})` rather than `f(x::Vararg{<:T})` or `f(x::Vararg{T}) where T` instead of `f(x::Vararg{T} where T)`.
Stacktrace:
 [1] UnionAll(v::Any, t::Any)
   @ Core ./boot.jl:299
 [2] top-level scope
   @ ~/.julia/packages/MLJBase/5aKTB/src/interface/data_utils.jl:133
 [3] include(mod::Module, _path::String)
   @ Base ./Base.jl:557
 [4] include(x::String)
   @ MLJBase ~/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:1
 [5] top-level scope
   @ ~/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:149
 [6] include
   @ ./Base.jl:557 [inlined]
 [7] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::String)
   @ Base ./loading.jl:2881
 [8] top-level scope
   @ stdin:6
in expression starting at /build/.julia/packages/MLJBase/5aKTB/src/interface/data_utils.jl:133
in expression starting at /build/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:1
in expression starting at stdin:6
ERROR: LoadError: Failed to precompile MLJBase [a7f614a8-145f-11e9-1d2a-a57a1082229d] to "/build/.julia/compiled/v1.11/MLJBase/jl_KDRdpO".
...

@devmotion devmotion mentioned this pull request Mar 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MilesCranmer MilesCranmer MilesCranmer left review comments

@OkonSamuel OkonSamuel Awaiting requested review from OkonSamuel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

selectcols should handle tuple input
3 participants
@ablaom @MilesCranmer @devmotion