Add FreezableLookup which replaces several lookup tables. #29
Conversation
danieldk
force-pushed
the
lookups
branch
2 times, most recently
from
ef41f66 to
f6cef40
Compare
|
|
||
| match &self.0 { | ||
| Fresh(cell) => cell.borrow_mut().add(t), | ||
| Frozen(numberer) => numberer.number(t).unwrap_or(0), |
There was a problem hiding this comment.
Fixing unknown to 0 means that padding positions need explicit assignment of different values (Tensor initializes with 0 by default).
There was a problem hiding this comment.
I see this as something that the user of lookups should decide, since there are also uses of lookups where there is no padding, such as classes in non-sequence learning tasks. 0 is just a sentinel for 'the value is not present in the lookup'. If this is confusing, maybe lookup should should have start_at as well and this method should return Option for values that are absent.
There was a problem hiding this comment.
I'd prefer the verbose option with returning an Option, as numberer returns an Option anyway. This way the user gets to call unwrap_or and decides which value to assign.
This change introduces a single lookup table that replaces: - TransitionLookup - LookupTable - MutableLookupTable StoredLookupTable is simplified, but still exists as a wrapper around FreezableLookup to simplify serialization. FreezableLookup implements three traits: - LookupValue: defines the methods to look up an identifier for a value and vice versa. - LookupLen: defines the `len` method. - Lookup: inherits LookupValue and LookupLen and is implemented for any type that implements both traits. In my first iteration, I had a single lookup trait. However, this sometimes led to type inference problems when the `len` method was used. Since `len` does not use the borrowed type in its signature, the type parameter for Lookup<B> could not be inferred. So, instead, we make the `len` method part of a trait without type parameters. Fixes #19.
|
I will close this PR, dpar is currently not really my focus. I'll try to do it sometime later when there is time. |
This change introduces a single lookup table that replaces:
StoredLookupTable is simplified, but still exists as a wrapper around
FreezableLookup to simplify serialization.
FreezableLookup implements three traits:
and vice versa. In contrast to the lookups that are replaced, values are
borrowed during lookup and only cloned upon insertion.
lenmethod.type that implements both traits.
In my first iteration, I had a single lookup trait. However, this
sometimes led to type inference problems when the
lenmethod was used.Since
lendoes not use the borrowed type in its signature, the typeparameter for
Lookup<B>could not be inferred. So, instead, we make thelenmethod part of a trait without type parameters.Fixes #19.