-
Notifications
You must be signed in to change notification settings - Fork 217
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 interface for ConstFsa #45
Conversation
k2/csrc/fsa.h
Outdated
|
||
int32_t NumFsas() { return num_fsas; } | ||
|
||
Cfsa &&operator [] (int32_t f) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it return an rvalue reference
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that was when you wanted to move
the data? So there aren't unnecessary constructors? I may misunderstand.
k2/csrc/fsa.h
Outdated
int32_t num_fsas; | ||
|
||
// The raw underlying data | ||
int32_t *data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we disable its copy constructor and assignment operator?
@danpovey |
Fantastic, and great timing!!!
Yes I think I have finished.
Let me push some other changes about DenseFsa, too.
…On Sat, May 30, 2020 at 8:01 PM Fangjun Kuang ***@***.***> wrote:
@danpovey <https://github.com/danpovey>
Have you finished the interface? I would like to write the implementation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/45#issuecomment-636321223>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOYLPQFMIGJWDLWQ53TRUDYSNANCNFSM4NJMG7OA>
.
|
BTW, I'm starting to wish we had some kind of Python interface. You'll see
in ConstFsa how it's supposed to interact with Python.. we'll mostly be
dealing with FSAs that are actually stored as Tensors, and have functions
defined in C++ that allow us to invoke FSA algorithms in parallel over
these things defined as Tensors (i.e. a single memory region storing
multiple FSAs, linearized).
We can figure out later where to have the Python/C++ boundary though. For
now we could wrap lots of stuff and whenever something becomes a
bottleneck, put it to C++.
…On Sat, May 30, 2020 at 9:32 PM Daniel Povey ***@***.***> wrote:
Fantastic, and great timing!!!
Yes I think I have finished.
Let me push some other changes about DenseFsa, too.
On Sat, May 30, 2020 at 8:01 PM Fangjun Kuang ***@***.***>
wrote:
> @danpovey <https://github.com/danpovey>
> Have you finished the interface? I would like to write the implementation.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/danpovey/k2/pull/45#issuecomment-636321223>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZFLOYLPQFMIGJWDLWQ53TRUDYSNANCNFSM4NJMG7OA>
> .
>
|
@danpovey |
I'd rather not depend on torch::Tensor in our actual C++ code itself.
My plan was to arrange things so our C++ code can tell us how much memory
we need to allocate, then we allocate
a Tensor with the right number of elements and our C++ code writes to it.
We could probably use the buffer protocol to
get the shape and data-pointer at the C++-wrapping-to-python level.
…On Sat, May 30, 2020 at 9:49 PM Fangjun Kuang ***@***.***> wrote:
@danpovey <https://github.com/danpovey>
As for the Python interface, should we depend on torch::Tensor in c++
directly?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/45#issuecomment-636333562>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOZCBDT7VKOHQNDFATDRUEFGNANCNFSM4NJMG7OA>
.
|
.. look at GetCfsaVecSize() for an example.
…On Sat, May 30, 2020 at 10:01 PM Daniel Povey ***@***.***> wrote:
I'd rather not depend on torch::Tensor in our actual C++ code itself.
My plan was to arrange things so our C++ code can tell us how much memory
we need to allocate, then we allocate
a Tensor with the right number of elements and our C++ code writes to it.
We could probably use the buffer protocol to
get the shape and data-pointer at the C++-wrapping-to-python level.
On Sat, May 30, 2020 at 9:49 PM Fangjun Kuang ***@***.***>
wrote:
> @danpovey <https://github.com/danpovey>
> As for the Python interface, should we depend on torch::Tensor in c++
> directly?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/danpovey/k2/pull/45#issuecomment-636333562>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZFLOZCBDT7VKOHQNDFATDRUEFGNANCNFSM4NJMG7OA>
> .
>
|
thanks, I see. |
There are conflicts now. Could you please resolve them and then merge? |
Merging now. If there are any problems, they can be fixed later. |
No description provided.