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 DataPointLike trait #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

0x8f701
Copy link

@0x8f701 0x8f701 commented Dec 16, 2020

Signed-off-by: Cheng JIANG [email protected]

Hi I added DataPointLike trait, I'd like to remove Copy bound but have no idea on how to do it.

Signed-off-by: Cheng JIANG <[email protected]>
@jeromefroe
Copy link
Owner

Hi @GopherJ! Thanks for the PR! Code changes look good to me, I'm just curious what the motivation for the PR is, is it so users can implement the DataPointLike trait for their own types and call the lttb function with it? That seems like it would be a great improvement to me.

On a side note, what do you think of renaming DataPointLike to Coordinates? The latter just seems cleaner to me but I'm curious what you think?

Off the top of my head, I think if we want to remove the Copy bound we'll need to change the lttb function to consume the input vector. Currently, we just copy values from the input vector into the output vector (e.g. sampled.push(data[a]);). If, on the other hand, we used a for loop to consume the input vector as we iterate over it we wouldn't need the Copy bound because the input vector will no longer be accessible. Something like the following perhaps:

for (i, v) in data.into_iter().enumerate() {
    if i == 0 {
        // Always add the first point.
        sampled.push(v);
    }

    // ...
}

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 this pull request may close these issues.

2 participants