rewrite pybind interface for Array2 and Fsa

[qindazhu]

Guys, please have a review. This PR is relatively important, if the whole structure/style is fine, I will follow it to wrap other algorithms to python (most of them will be straightforward).

Note as in Python, we cannot pass primitive type (int, float, etc.) to function as reference/pointer, I get Array1 back to support those arc_map (see example in arc_sort)

[csukuangfj]

You can leave the interfaces as they are.

To wrap them to python, you only need to add an indirection.

Take

void ArcSorter::GetOutput(Fsa *fsa_out, int32_t *arc_map /*= nullptr*/) const

as an example, you can use

pyclass.def("get_output", [](const ArcSorter& self, Fsa* fsa_out, std::vector<int32_t>* arc_map = nullptr) {
        self.GetOutput(fsa_out, arc_map ? arc_map->data() : nullptr);
}, py::arg("fsa_out"), py::arg("arc_map") = nullptr
);

I assume that you have made std::vector<int32_t> an opaque type and arc_map is saved in std::vector<int32_t>.

[qindazhu]

I make Array1 share memory with torch.tensor as we do in Array2, so does vector<int32_t> here share memory with tensor? how do you do that with above code?

[csukuangfj]

I make Array1 share memory with torch.tensor as we do in Array2, so does vector<int32_t> here share memory with tensor? how do you do that with above code?

You can still leave the interfaces as they are. Just replace std::vector<int32_t> above with Array1.

[csukuangfj]

Alternatively, you can replace std::vector<int32_t> with a py::capsule and construct an Array1 from the py::capsule inside the lambda function. In this case, only DLPackArray1 is needed.

[qindazhu]

Oh, you're right, as we could just change the interface in Python code.

You can still leave the interfaces as they are. Just replace std::vector<int32_t> above with Array1.

I would like to choose this one as constructing Array1 outside of the function is clearer and consistent with what we do for Fsa.

[csukuangfj]

I would like to choose this one as constructing Array1 outside of the function is clearer and consistent with what we do for Fsa.

Anyway, there is no need to change k2/csrc/*.cc files.

[qindazhu]

Updated, thanks @csukuangfj for the suggestions.

[csukuangfj]

+1

[danpovey]

Thanks for the review Fangjun! It's OK with me. I'm not doing a detailed review as I want to focus on figuring out some next-steps stuff.

…

On Wed, Jul 22, 2020 at 8:47 PM Fangjun Kuang ***@***.***> wrote: +1 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/danpovey/k2/pull/69#issuecomment-662432514>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZFLO2E6JKJ3MPU6OCNMWTR43NVZANCNFSM4PETU43A> .

[qindazhu]

Merging..