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 Box impl #41

Merged
merged 2 commits into from
Jul 22, 2021
Merged

Add Box impl #41

merged 2 commits into from
Jul 22, 2021

Conversation

ava57r
Copy link
Collaborator

@ava57r ava57r commented Jul 20, 2021

No description provided.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

I am not opposed to this, but the native index objects are already heap allocated and thus already "sort of" boxed. With this we are mostly creating a second dereferencing step. Is there a use case for this?

src/index/mod.rs Outdated Show resolved Hide resolved
@ava57r
Copy link
Collaborator Author

ava57r commented Jul 20, 2021

For erase type of index.

Rust std.
https://doc.rust-lang.org/beta/std/boxed/struct.Box.html#impl-Iterator

For code:

let mut v: Vec<Box<dyn faiss::Index>> = vec![];

let f1 = FlatIndexImpl::new();
v.push(f1);

let f2 = IndexImpl::new();
v.push(f2);

@Enet4
Copy link
Owner

Enet4 commented Jul 20, 2021

What if we also had a way to upcast a specific index type back to an IndexImpl?

let f1 = FlatIndex::new_l2(128);
let f2 = index_factory(128, "Flat", MetricType::L2);
let v: Vec<faiss::IndexImpl> = vec![
    f1.upcast(),
    f2,
];

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Looks OK to me nevertheless.

@ava57r
Copy link
Collaborator Author

ava57r commented Jul 20, 2021

What if we also had a way to upcast a specific index type back to an IndexImpl?

let f1 = FlatIndex::new_l2(128);
let f2 = index_factory(128, "Flat", MetricType::L2);
let v: Vec<faiss::IndexImpl> = vec![
    f1.upcast(),
    f2,
];

I agree.

Box<dyn faiss::Index> is Rust idiomatic path.

@ava57r
Copy link
Collaborator Author

ava57r commented Jul 22, 2021

What if we also had a way to upcast a specific index type back to an IndexImpl?

let f1 = FlatIndex::new_l2(128);
let f2 = index_factory(128, "Flat", MetricType::L2);
let v: Vec<faiss::IndexImpl> = vec![
    f1.upcast(),
    f2,
];

Do upcast in C_API?

@Enet4 Enet4 merged commit 4aba00d into Enet4:master Jul 22, 2021
@Enet4
Copy link
Owner

Enet4 commented Jul 22, 2021

Do upcast in C_API?

I don't imagine this to be necessary. The operation is a reinterpret_cast between two index pointers, which is a no-op and can be done in Rust. Having functions for a downcast was important because then we could dynamic_cast, which checks at run-time that the target type is acceptable.

@ava57r ava57r deleted the box-index branch July 22, 2021 08:31
@ava57r ava57r mentioned this pull request Jul 22, 2021
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