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.

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

deprecate delete!, define deleteat! #2854

Merged
merged 6 commits into from
Nov 14, 2021
Merged

deprecate delete!, define deleteat! #2854

merged 6 commits into from
Nov 14, 2021

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Sep 2, 2021

Fixes #2853

@nalimilan: the issue I was not sure about what would be best to do is if passed indices contain duplicates or are not sorted.
Intuitively I would prefer deleteat! to perform sorting and deduplication instead of throwing an error (the cost of this should not be super big in typical cases). Alternatively I could require in keepat! sorted and unique indices.

@bkamins bkamins added breaking The proposed change is breaking. feature labels Sep 2, 2021
@bkamins bkamins added this to the 1.3 milestone Sep 2, 2021
@nalimilan
Copy link
Member

Thanks. It's indeed a bit weird to have different requirements for deleteat! and keepat!. How costly is sorting and deduplication (when it's needed but more importantly when the input is already unique and sorted)? Do you have any idea whether implementing keepat! by calling keepat! on columns would make a difference for performance?

@bkamins
Copy link
Member Author

bkamins commented Sep 2, 2021 •
edited
Loading

Do you have any idea whether implementing keepat! by calling keepat! on columns would make a difference for performance?

For this keepat! would have to be made available for vectors on all supported Julia versions. I would do this this way assuming JuliaLang/Compat.jl#750 is approved for adding. I would switch to using keepat! then as it should be faster.

@nalimilan
Copy link
Member

I guess it will be accepted in Compat, but we'll have to do it ourselves if we want it to happen.

@bkamins
Copy link
Member Author

bkamins commented Sep 4, 2021

OK - I will implement keepat! in Compat.jl when its design is settled in Julia Base (which means probably after Julia 1.7 release) then get back to this issue.

@bkamins
Copy link
Member Author

bkamins commented Sep 9, 2021

we should also add insert! method. I wait with this PR till keepat! is stabilized in Julia 1.7.

@bkamins
Copy link
Member Author

bkamins commented Sep 24, 2021

Also add splice!.

@bkamins bkamins changed the title deprecate delete!, define deleteat! and keepat! deprecate delete!, define deleteat! Nov 14, 2021
@bkamins bkamins removed the breaking The proposed change is breaking. label Nov 14, 2021
@bkamins
Copy link
Member Author

bkamins commented Nov 14, 2021

@nalimilan - I have decided to split keepat! to a separate issue (#2936). This PR is just about cleaning up delete! and deleteat!. Notably the fact that keepat! is missing is not a huge problem as one can easily do deleteat!(df, Not(inds)).

This PR should be good for a review.

nalimilan
nalimilan reviewed Nov 14, 2021
View reviewed changes
test/dataframe.jl Outdated
Comment on lines 692 to 695
# the exception type changed between Julia 1.0.2 and Julia 1.1
# so we use their supertype below
@test_throws Exception keepat!(df, [10])
@test_throws Exception keepat!(df, [0])
Copy link
Member

Choose a reason for hiding this comment

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

Make this conditional on VERSION? That way we ensure the type doesn't change without us noticing.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I will add it. I was just lazy because tired of fixing such things since Julia Base is not considering changing of exception type as breaking.

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
src/deprecated.jl Show resolved Hide resolved
@bkamins
Copy link
Member Author

bkamins commented Nov 14, 2021

This should be good for another review.

@bkamins
Copy link
Member Author

bkamins commented Nov 14, 2021

Thank you!

@bkamins bkamins merged commit d00b5d2 into main Nov 14, 2021
@bkamins bkamins deleted the bk/deprecate_delete branch November 14, 2021 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

delete! in DataFrames.jl
2 participants
@bkamins @nalimilan