search

ufal / treex

Treex NLP framework
33 stars 6 forks source link

Switching to bidirectional alignment in AttributeParameterized #46

Closed tuetschek closed 8 years ago

tuetschek commented 8 years ago

This uses the Treex::Tool::Align::Utils module by @michnov to provide bidirectional alignment – i.e. all nodes aligned from and to the current node will be returned for aligned->.

It works for me and I think it's better like this, but perhaps @ptakopysk may have a problem with it? Actually, @ptakopysk, could you use this to get rid of the $alignment_hash variable? It would make the code much nicer...

martinpopel commented 8 years ago

@michnov What about moving all alignment-related methods to a role Treex::Core::Node::Aligned and adding there (as node methods) also the three documented functions from Treex::Tool::Align::Utils? (This is not really related to this PR, sorry.)

ptakopysk commented 8 years ago

Thanks, I will check this out, it could make my code much nicer. Always having to take care about the direction of the links used to bother me a lot, and has been source of errors multiple times. Actually, @varisd may be affected, as he is the one playing with Depfix/MLFix now.

tuetschek commented 8 years ago

@martinpopel : Are there types of nodes that wouldn't have the role Aligned, if we created it?

martinpopel commented 8 years ago

@tuetschek: I think all kinds of nodes can be aligned (although no one used it for n-nodes as fas as I know). So the role can be consumed by Treex::Core::Node. I though that having all the alignment-related methods in a separate file could be handy.

tuetschek commented 8 years ago

@martinpopel: OK, you are probably right.