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

make recode! type stable #407

Merged
merged 17 commits into from
Jan 3, 2025
Merged

make recode! type stable #407

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1db6279
make recode! type stable
tiemvanderdeure Nov 1, 2024
ef10966
move optimize_pair away from main function
tiemvanderdeure Nov 20, 2024
1688fb2
improve type stability of recode_in
tiemvanderdeure Nov 20, 2024
b412ce4
un-inline recode_in
tiemvanderdeure Nov 20, 2024
0581607
pass first and last pairs as tuples
tiemvanderdeure Nov 22, 2024
06b9265
recode_in is always false for identical types
tiemvanderdeure Nov 22, 2024
74123a6
revert changes to recode_in, inline findfirst
tiemvanderdeure Nov 28, 2024
15a09db
do not splat pairs passed to `_recode!`
tiemvanderdeure Nov 28, 2024
c419cf5
require julia 1.6
tiemvanderdeure Nov 28, 2024
8e2db47
Revert "require julia 1.6"
tiemvanderdeure Nov 28, 2024
2a446da
define inlined findfirstrecode
tiemvanderdeure Nov 28, 2024
445469e
use `Compat.@inline`
tiemvanderdeure Dec 30, 2024
91de203
avoid `isnothing`
tiemvanderdeure Dec 31, 2024
539a3ba
allow compat v4
tiemvanderdeure Jan 2, 2025
25af087
various minor changes
tiemvanderdeure Jan 2, 2025
b08a4ba
drop second import Compat
tiemvanderdeure Jan 3, 2025
04695fa
whitespace and comments
tiemvanderdeure Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/recode.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,11 @@ function recode!(dest::AbstractArray{T}, src::AbstractArray, default::Any, pairs
throw(DimensionMismatch("dest and src must be of the same length (got $(length(dest)) and $(length(src)))"))
end

opt_pairs = map(optimize_pair, pairs)

@inbounds for i in eachindex(dest, src)
x = src[i]

for j in 1:length(opt_pairs)
p = opt_pairs[j]
for j in 1:length(pairs)
p = optimize_pair(pairs[j])
Copy link
Member

Choose a reason for hiding this comment

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

This looks problematic when pairs[j] is an array as it will create a Set for each item. Have you investigated why map(optimize_pair, pairs) isn't inferred correctly? In theory it should, and I think I had carefully checked performance when I wrote that function so I'm surprised it's not the case.

Copy link
Contributor Author

@tiemvanderdeure tiemvanderdeure Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

pairs[j] is never an array, it is always a Pair. I just got rid of the map and moved it inside the loop, but it still iterates over the exact same thing, so I don't think that should ever be problematic.

I was also surprised that map(optimize_pair, pairs) wasn't inferred, some change in the compiler must have triggered this. I stumbled into this easy fix, so then I didn't investigate further.

Copy link
Member

Choose a reason for hiding this comment

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

I mean when the first element is an array to that the call hits optimize_pair(pair::Pair{<:AbstractArray}).

Copy link
Member

@nalimilan nalimilan Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that currently we create a Set once for each pair, and with this change we would create it for each pair for each entry in the input array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's in the nested loop. I don't know how I missed that.

# we use isequal and recode_in because we cannot really distinguish scalars from collections
if x ≅ p.first || recode_in(x, p.first)
dest[i] = p.second
Expand Down Expand Up @@ -101,9 +99,7 @@ function recode!(dest::CategoricalArray{T}, src::AbstractArray, default::Any, pa
throw(DimensionMismatch("dest and src must be of the same length (got $(length(dest)) and $(length(src)))"))
end

opt_pairs = map(optimize_pair, pairs)

vals = T[p.second for p in opt_pairs]
vals = T[p.second for p in pairs]
tiemvanderdeure marked this conversation as resolved.
Show resolved Hide resolved
default !== nothing && push!(vals, default)

levels!(dest.pool, filter!(!ismissing, unique(vals)))
Expand All @@ -117,8 +113,8 @@ function recode!(dest::CategoricalArray{T}, src::AbstractArray, default::Any, pa
@inbounds for i in eachindex(drefs, src)
x = src[i]

for j in 1:length(opt_pairs)
p = opt_pairs[j]
for j in 1:length(pairs)
p = optimize_pair(pairs[j])
# we use isequal and recode_in because we cannot really distinguish scalars from collections
if x ≅ p.first || recode_in(x, p.first)
drefs[i] = dupvals ? pairmap[j] : j
Expand Down
Loading