Replace Numberer by Transitions in the transition systems. #11
Conversation
There was a problem hiding this comment.
I like the solution with the two variants to avoid the indirection through the box at inference time. I am wondering though why it is a specific struct for transitions. In features::lookup there exist similar structs that could be merged with this implementation. If I didn't miss something Transitions is already generic over the same traits Numberer constrains and does not limit itself to structs that implement the Transition trait.
For future consideration: provide a thaw method as well?
I'd say as long as this struct is used for Transitions only it is probably unproblematic to use one-way freeze
dpar/src/system/trans_system.rs
Outdated
| /// the table becomes immutable. Lookups of transitions that are not in the | ||
| /// table will result in a special index (`0`). | ||
| #[derive(Deserialize, Eq, PartialEq)] | ||
| pub enum Transitions<T> |
There was a problem hiding this comment.
TransitionLookupTable might be a more descriptive name, with Transitions::xxx I'd expect transitions and not a lookup-table. (TransitionTable is unfortunately ambiguous too)
There was a problem hiding this comment.
Ok, I'll do a big rename tomorrow. Thanks for the review!
|
I am wondering though why it is a specific struct for transitions. In features::lookup there exist similar structs that could be merged with this implementation. I agree. I considered doing that. However, table lookups from I can create an issue for merging |
TransitionLookup is a wrapper of Numberer that has several additional properties that are useful for transition tables. - It insures that the identifier 0 for unknown transitions. - It returns the correct length of the transition table, that includes the special identifier 0. - The table can be both fresh and frozen. A fresh table automatically adds transitions that are not known. A frozen table returns the special identifier 0 when a transition is now known. For future consideration: provide a thaw method as well?
|
Changes:
|
| /// If the the transition is not in the table, it is added for a | ||
| /// fresh table. Frozen tables will return the special identifier | ||
| /// `0` in such cases. | ||
| pub fn lookup(&self, t: T) -> usize { |
There was a problem hiding this comment.
Ownership of t is only required in case of a fresh lookup table where t is added to the numberer. Again, when dealing with Transition Enums probably not really a problem. If we should generalize the struct later on to e.g. number strings we might consider adding to_owned or clone to the trait bounds so that we can pass in a &T and only convert it when necessary.
There was a problem hiding this comment.
If we should generalize the struct later on to e.g. number strings we might consider adding to_owned or clone to the trait bounds so that we can pass in a &T and only convert it when necessary.
I agree that this would be nice. IIRC I decided use moves in Numberer::add to avoid two lookups when the value is absent and HashMaps entry method requires an owned value. But since in transition and feature lookups, the lookups succeed in the vast majority of cases, doing an get (with a reference) + insert when absent would be more economical. There was an RFC to make entry work with borrowed values, but it seems to be stranded:
I'll add this as a todo item to the 'one lookup to rule them all'-issue.
There was a problem hiding this comment.
We could also consider moving the numberer / lookup into a separate crate.
For the lemmatizer, I had to implement the same functionality and dealt with similar issues. If there'd be a crate we might spare some people a lot of unnecessary worries and work.
Transitions is a wrapper of Numberer that has several additional
properties that are useful for transition tables.
the special identifier 0.
adds transitions that are not known. A frozen table returns the
special identifier 0 when a transition is now known.
For future consideration: provide a thaw method as well?