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

NaNs Are Returned by non NaN Downsamplers #73

Open
mike-iqmo opened this issue May 15, 2024 · 1 comment · May be fixed by #83
Open

NaNs Are Returned by non NaN Downsamplers #73

mike-iqmo opened this issue May 15, 2024 · 1 comment · May be fixed by #83

Comments

@mike-iqmo
Copy link

mike-iqmo commented May 15, 2024

When using the non NaN version of the samplers, I am getting NaNs in the downsampled data.

My understanding from the documentation was that MinMaxLTTBDownsampler would omit all NaN values.

Some code demonstrating this below

n=10_000
y = np.arange(n, dtype=np.float64)
for i in range(1,100):
    y[i+100] = np.nan

sampled=MinMaxLTTBDownsampler().downsample(y,n_out=1000)
print(f"MinMaxLTTBDownsampler:{[i for i in sampled if np.isnan(y[i])]}")

sampled_nan=NaNMinMaxLTTBDownsampler().downsample(y,n_out=1000)
print(f"NaNMinMaxLTTBDownsampler:{[i for i in sampled_nan if np.isnan(y[i])]}")

That will print

MinMaxLTTBDownsampler:[101, 111, 121, 131, 141, 151, 161, 171, 181, 191]
NaNMinMaxLTTBDownsampler:[101, 111, 121, 131, 141, 151, 161, 171, 181, 191]
@mike-iqmo mike-iqmo changed the title NaNs Are Always Returned NaNs Are Returned by non NaN Downsamplers May 15, 2024
@jvdd jvdd linked a pull request Jan 31, 2025 that will close this issue
@jvdd
Copy link
Member

jvdd commented Jan 31, 2025

Good catch! The code didn't handle the case where the bin is full of NaNs (as is the case in your example).

#83 now prunes NaN indices after downsampling (in Rust) for the omit NaN downsamplers

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 a pull request may close this issue.

2 participants