Aux labels plus notes on Python interface #29
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qindazhu
reviewed
May 6, 2020
notes/python.txt
Outdated
|
|
||
| # For composition we can rely on PyTorch's inbuilt autograd | ||
| def Compose(a: FsaVec, a_weights: Tensor, | ||
| a: FsaVec, b_weights: Tensor): |
There was a problem hiding this comment.
I guess it is b:FsaVec, b_weights:Tensor
qindazhu
reviewed
May 6, 2020
notes/python.txt
Outdated
| # Composing with transducers. Assumes that A is an acceptor but B may | ||
| # have auxiliary symbols. | ||
| def TransducerCompose(a: FsaVec, a_weights: Tensor, | ||
| a: FsaVec, b_weights: Tensor, |
There was a problem hiding this comment.
again, I guess it is b:FsaVec
qindazhu
reviewed
May 6, 2020
notes/python.txt
Outdated
| class TotalWeight(Function): | ||
| """ | ||
| Returns the total weight of FSAs (i.e. the log-sum-exp across | ||
| paths) as a Tensor with shapee (num_fsas,) |
qindazhu
reviewed
May 6, 2020
k2/csrc/aux_labels.h
Outdated
| @@ -50,28 +50,28 @@ struct AuxLabels { | |||
| /* | |||
| Maps auxiliary labels after an FSA operation where each arc in the output | |||
| FSA corresponds to exactly one arc in the input FSA. | |||
| @param [in] labels_in Labels on the arcs of the input FSA | |||
| @param [in] labels_in int32_ts on the arcs of the input FSA | |||
There was a problem hiding this comment.
the int32_ts means the plural of int32_t? (just a question as I really feel hard to get the meaning of Auxint32_ts from its name)
csukuangfj
reviewed
May 6, 2020
k2/csrc/aux_labels.h
Outdated
| @@ -34,7 +34,7 @@ namespace k2 { | |||
| This allows you to store auxiliary labels (e.g. olabels or ilabels) | |||
| on each arc of an Fsa. | |||
| */ | |||
| struct AuxLabels { | |||
| struct Auxint32_ts { | |||
There was a problem hiding this comment.
This does not follow ClassNamingStyle.
csukuangfj
reviewed
May 6, 2020
| // This comparator function compares the weights, but is careful in case of | ||
| // ties to ensure deterministic behavior. | ||
| bool operator < (const DetStateElement &other) const { | ||
| if (weight < other.weight) return true; |
There was a problem hiding this comment.
What is the return value if weight == other.weight ??
There was a problem hiding this comment.
Sorry, I didn't realize I had included this code, it is not finished. I have to get back to this.
csukuangfj
reviewed
May 6, 2020
k2/csrc/determinize.cc
Outdated
| } | ||
| } | ||
|
|
||
| size_t size() const { return cur_output_state_; } |
csukuangfj
reviewed
May 6, 2020
k2/csrc/fsa.h
Outdated
| language models. Actually we'll template on types like this. There is no | ||
| need to actually inherit from this class. */ | ||
| class DeterministicGenericFsa { | ||
|
|
csukuangfj
reviewed
May 6, 2020
| int32_t Start(); | ||
|
|
||
|
|
||
| bool LookupArc(int32_t cur_state, |
There was a problem hiding this comment.
Why are there no const modifiers for Get ?
There was a problem hiding this comment.
Because this is an interface for a possibly dynamic object.
qindazhu
reviewed
May 6, 2020
| private: | ||
|
|
||
| int32_t cur_output_state_ { 0 }; | ||
| std::unordered_map<std::pair<uint64_t, uint64_t>, int32_t, DetStateVectorHasher> map_; |
There was a problem hiding this comment.
Yeah, might do that. This code needs to be finished.
csukuangfj
reviewed
May 6, 2020
k2/csrc/determinize.cc
Outdated
| @@ -0,0 +1,230 @@ | |||
| // k2/csrc/fsa_algo.cc | |||
There was a problem hiding this comment.
The filename should be changed.
csukuangfj
reviewed
May 6, 2020
| DetStateElement *elem = d.head; | ||
| int32_t seq_len = d.seq_len; | ||
| for (int32_t i = 0; i < seq_len; i++) { | ||
| a = elem->symbol + 102299 * a; |
There was a problem hiding this comment.
Is there an error inside the for loop?
elem is never updated.
It should be ++i, not i++.
|
I will fix these issues, but best to look again much later when that code
is finished.
Let's merge anyway though- I don't want to go to the trouble of removing
this part just yet.
…On Wed, May 6, 2020 at 12:53 PM Fangjun Kuang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In k2/csrc/determinize.cc
<https://github.com/danpovey/k2/pull/29#discussion_r420544317>:
> + is one of those lifetime-of-the-universe type of things (kind of like
+ the theoretical potential for git hash collision) that we ignore. */
+ void DetStateToCompact(const DetState &d,
+ std::pair<uint64_t, uint64_t> *vec) {
+ assert(d.normalized);
+
+ uint64_t a = d.base_state + 17489 * d.seq_len,
+ b = d.base_state * 103979 + d.seq_len;
+
+ // We choose an arbitrary DetStateElement (the first one in the list) to
+ // read the symbol sequence from; the symbol sequence will be the same no
+ // matter which element we choose to trace back.
+ DetStateElement *elem = d.head;
+ int32_t seq_len = d.seq_len;
+ for (int32_t i = 0; i < seq_len; i++) {
+ a = elem->symbol + 102299 * a;
Is there an error inside the for loop?
elem is never updated.
------------------------------
It should be ++i, not i++.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/29#pullrequestreview-406302924>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO5XCGKWBMIW4GAVONDRQDUM3ANCNFSM4MZRKCHA>
.
|
|
Merging to avoid future conflicts, although not everything is resolved here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.