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

add resize!, keepat!, pop!, popfist!, popat! #3047

Merged
merged 20 commits into from
Jun 6, 2022
Merged

add resize!, keepat!, pop!, popfist!, popat! #3047

merged 20 commits into from
Jun 6, 2022

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented May 8, 2022

Partially implement changes requested in #2936.

@nalimilan - if you agree with the proposed changes (especially deleteat! - I think, given that the operation is expensive anyway, we can afford sorting and de-duplicating incorrect input instead of having an undefined behavior in this case) I will add tests.

@bkamins bkamins added the feature label May 8, 2022
@bkamins bkamins added this to the 1.4 milestone May 8, 2022
src/DataFrames.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
@bkamins bkamins changed the title add keepat!, pop!, popfist!, popat!, and make deleteat! more flexible add keepat!, pop!, popfist!, popat! May 24, 2022
@bkamins bkamins changed the title add keepat!, pop!, popfist!, popat! add resize!, keepat!, pop!, popfist!, popat! Jun 3, 2022
@bkamins
Copy link
Member Author

bkamins commented Jun 3, 2022

@nalimilan - this PR is ready for review. Thank you!

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
test/dataframe.jl Outdated Show resolved Hide resolved
test/dataframe.jl Outdated Show resolved Hide resolved
test/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
src/dataframe/dataframe.jl Show resolved Hide resolved
throw(ArgumentError("invalid index of type Bool"))
end

if !(eltype(inds) <: Integer || all(x -> x isa Integer, inds))
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

Suggested change
if !(eltype(inds) <: Integer || all(x -> x isa Integer, inds))
# this is required because of https://github.com/JuliaData/InvertedIndices.jl/issues/31
if !(eltype(inds) <: Integer || all(x -> x isa Integer, inds))

Copy link
Member Author

Choose a reason for hiding this comment

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

No, here it is "just required". Here we know that inds is AbstractVector so it is not connected with Not. We just need to make sure all elements are Integer, as otherwise in line 826 in general user could get a weird error that <= is not defined for values passed.

I know it adds some performance overhead, but this method anyway is not type stable, so I think it is OK to add this check.

bkamins and others added 2 commits June 5, 2022 20:21
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins bkamins merged commit fec65bf into main Jun 6, 2022
@bkamins bkamins deleted the bk/add_resize! branch June 6, 2022 09:45
@bkamins
Copy link
Member Author

bkamins commented Jun 6, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants