Remove cycles from the dependency graph #16
Conversation
| /// Break cycles in the graph. | ||
| /// | ||
| /// Panics when a token does not have a head. | ||
| pub fn break_cycles(sent: &mut Sentence, root_idx: usize) { |
There was a problem hiding this comment.
I think almost all other methods return Errors to indicate failures, why panic here? From what I can tell, the calling method decode returns an Error, too.
There was a problem hiding this comment.
encode/decode return errors for invalid user input. This is a private function and having a head on each token is an invariant. Passing a graph with headless tokens to break_cycles is a programming error and should thus lead to an assertion-type panic and not to a regular error that is communicated to the user.
I have extended the rustdoc a bit that attach_orphans should be called before this function if the graph has headless tokens.
There was a problem hiding this comment.
Didn't catch that the post_processing module was crate-private, I only looked at the pub fn break_cycles signature.
There was a problem hiding this comment.
Ok, I'll merge this then.
| pub fn break_cycles(sent: &mut Sentence, root_idx: usize) { | ||
| loop { | ||
| let components = { | ||
| let digraph: &DiGraph<_, _> = (&*sent).into(); |
There was a problem hiding this comment.
This looks somewhat ugly, to say the least. Perhaps a .graph() method on Sentence would make for some nicer looking code? I don't remember if we discussed that in the issues at the conllx-rs repo.
let graph = sent.graph();or even more concise (untested, don't know if lifetimes are an issue here):
let components = tarjan_scc(sent.graph())
.into_iter()
.filter(|c| c.len() > 1)
.collect::<Vec<_>>();There was a problem hiding this comment.
This is indeed missing in conllx-rs. Another annoyance is that DepGraphMut does not have a method to remove edges (something like remove_deprel(dependent: usize)). I'll add issues.
What makes things less ergonomic in this case is that type-inference does understand the simple into() call, because sent is &mut and the From implementation is for &. Not providing an &mut implementation was intentional to avoid changing the graph from under our feet.
There was a problem hiding this comment.
Applies the strategy suggested by Strzyz et al., 2019 to attach the leftmost token in a cycle to the first root token.
Applies the strategy suggested by Strzyz et al., 2019 to attach the
leftmost token in a cycle to the first root token.