Create arc_indexes from arcs, add a duplicate of final state #27
Conversation
|
Final-state should always have the largest state number, yes.
…On Tue, May 5, 2020 at 1:41 PM Fangjun Kuang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In k2/csrc/fsa.h
<https://github.com/danpovey/k2/pull/27#discussion_r419874279>:
> std::vector<int32_t> arc_indexes;
// Note: an index into the `arcs` array is called an arc-index.
std::vector<Arc> arcs;
- StateId NumStates() const { return static_cast<StateId>(arc_indexes.size()); }
+ Fsa() {}
+ // just for creating testing FSA examples for now.
+ Fsa(std::vector<Arc> fsa_arcs, int32_t final_state) {
Shouldn't the final state *always* have the largest state number?
—
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/27#discussion_r419874279>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOYSRBMZKHJKDFFN2OLRP6RJXANCNFSM4MZI42BA>
.
|
Yes, but @csukuangfj 's question is really another question. We can't get final state from a non-co-accessible FSA. Just image an FSA with arc |
|
@csukuangfj do you notice that |
|
I prefer no braces for one-line things, i.e. wherever they are not needed.
It keeps the code shorter -> more readable on smaller screens.
…On Tue, May 5, 2020 at 1:54 PM Haowen Qiu ***@***.***> wrote:
@csukuangfj <https://github.com/csukuangfj> do you notice that checks now
are running with huge long time? Maybe we should remove some version ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/27#issuecomment-623868867>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOYNYGJL7QOFIWRI6ALRP6SYHANCNFSM4MZI42BA>
.
|
|
you can remove cppcheck. the average check time per request is 10 min. |
|
I agree with you (Actually I just agree on following consistent style) Ready to merge if there's no other issues. |
|
I see what you mean RE style.
But also I'm not sure that I want accessors for the members; and for those
function members, it does seem to be the natural place to put them.
Any ideas about the best way to handle it?
…On Tue, May 5, 2020 at 3:15 PM Fangjun Kuang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In k2/csrc/fsa.h
<https://github.com/danpovey/k2/pull/27#discussion_r419907054>:
> @@ -77,13 +79,43 @@ struct Fsa {
// contains the first arc-index leaving this state (index into `arcs`).
It is NOT a good style to define a struct in c++ that has both public
data members and public methods.
@qindazhu <https://github.com/qindazhu> @danpovey
<https://github.com/danpovey>
what do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/27#pullrequestreview-405541929>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO224PCP4YHZZA6UMIDRP64ITANCNFSM4MZI42BA>
.
|
|
Our current style is C (instead of C++/OOP that including fsa-related operations in a class, i.e.
|
|
I'm OK with that-- having a struct plus a few const members where helpful.
…On Tue, May 5, 2020 at 3:40 PM Haowen Qiu ***@***.***> wrote:
Our current style is C (instead of C++/OOP that including fsa-related
operations in a class, i.e. fsa.empty(), fsa.ArcSort()), so I tend to
keep Fsa structure very simple with only public members (and only a few
const method and consturctor).
I see what you mean RE style. But also I'm not sure that I want accessors
for the members; and for those function members, it does seem to be the
natural place to put them. Any ideas about the best way to handle it?
… <#m_-8687208855307822183_>
On Tue, May 5, 2020 at 3:15 PM Fangjun Kuang *@*.*> wrote: @.** commented
on this pull request. ------------------------------ In k2/csrc/fsa.h <#27
(comment) <https://github.com/danpovey/k2/pull/27#discussion_r419907054>>:
> @@ -77,13 +79,43 @@ struct Fsa { // contains the first arc-index leaving
this state (index into arcs). It is NOT a good style to define a struct
in c++ that has both public data members and public methods. @qindazhu
<https://github.com/qindazhu> https://github.com/qindazhu @danpovey
<https://github.com/danpovey> https://github.com/danpovey what do you
think? — You are receiving this because you were mentioned. Reply to this
email directly, view it on GitHub <#27 (review)
<https://github.com/danpovey/k2/pull/27#pullrequestreview-405541929>>, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAZFLO224PCP4YHZZA6UMIDRP64ITANCNFSM4MZI42BA
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/27#issuecomment-623904839>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO26Z44BWPJUUH23U4TRP67H7ANCNFSM4MZI42BA>
.
|
|
+2 |
|
Please merge if there's no other issues |
arc_indexesfromarcs.arc_indexesto avoid boundary check in many implentations.Please merge this PR ASAP before other PRs if there's no other issues as:
.ccfiles.arc_indexes.