search

ufal / treex

Treex NLP framework
33 stars 6 forks source link

Model interpol #7

Closed ptakopysk closed 9 years ago

ptakopysk commented 9 years ago

Refactored Tr?AddVariants (and related blocks) to support interpolation of multiple models in a simple way (see e.g. Treex::Block::T2T::EN2CS::TrFAddVariantsInterpol). One can now specify any number of models for interpolation, together with their type and weight, in the following way:

T2T::EN2CS::TrFAddVariantsInterpol models='maxent 0.5 formeme_czeng09.maxent.compact.pls.slurp.gz static 1.0 formeme_czeng09.static.pls.slurp.gz'

Besides, duplicated code has been factored out into TrBaseAddVariantsInterpol (and unnecessary BaseTrLAddVariants has been removed), resulting in a reduction of LOC, even though new functions were added. All original APIs and functionality were retained. The Tr?AddVariantsInterpol were created in the language-independent space, and subclassed for EN2CS and CS2EN.

tuetschek commented 9 years ago

By just looking at the code, it seems to be OK to me (and I assume you've tested it). Just the module naming is a little confusing to me (AddVariantsInterpol suggests something more sophisticated than just AddVariants, but is in fact a base class).

martinpopel commented 9 years ago

@tuetschek: If I understand it correctly, @ptakopysk kept the old block names (those without Interpol), so no change in scenarios is needed. I would say, we have two options: A) keep the support for old block names (and merge this pull request now), B) keep only the Interpol blocks and update all scenarios, at least those in https://github.com/ufal/treex/tree/master/lib/Treex/Scen/Transfer (unfortunately, the transfer scenarios for other languages are still kept as plain-text files, not versioned in this repo).

tuetschek commented 9 years ago

You are right -- and it's probably better not to change everything.

I was thinking about making the "Interpol" block names sound more "basic" than the default, but I cannot come up with anything better, so it's probably best to keep it as it is.

O. On Jul 13, 2015 12:11 PM, "Martin Popel" notifications@github.com wrote:

@tuetschek https://github.com/tuetschek: If I understand it correctly, @ptakopysk https://github.com/ptakopysk kept the old block names (those without Interpol), so no change in scenarios is needed. I would say, we have two options: A) keep the support for old block names (and merge this pull request now), B) keep only the Interpol blocks and update all scenarios, at least those in https://github.com/ufal/treex/tree/master/lib/Treex/Scen/Transfer (unfortunately, the transfer scenarios for other languages are still kept as plain-text files, not versioned in this repo).

— Reply to this email directly or view it on GitHub https://github.com/ufal/treex/pull/7#issuecomment-120882845.

ptakopysk commented 9 years ago

Thanks for merging. Actually, AddVariantsInterpol does indeed perform something more sophisticated than AddVariants, which is its subclass and only uses part of its full power. Technically, AddVariants only adds an API to AddVariantsInterpol (and some default values) -- so it could have been a Moose role (and that is indeed the actual implementation), but Moose roles cannot be parts of scenarios...