diff --git a/convokit/model/utteranceNode.py b/convokit/model/utteranceNode.py index 8dd7446fe..efb5402be 100644 --- a/convokit/model/utteranceNode.py +++ b/convokit/model/utteranceNode.py @@ -16,7 +16,11 @@ def __init__(self, utt: Utterance): self.children = [] def set_children(self, children: List["UtteranceNode"]): - self.children = sorted(children, key=lambda w: w.utt.timestamp) # earliest to latest utt + # earliest to latest utt; utterances with no timestamp (None) are sorted + # last and never compared against each other or against numeric timestamps + self.children = sorted( + children, key=lambda w: (w.utt.timestamp is None, w.utt.timestamp or 0) + ) def pre_order(self): """ diff --git a/convokit/tests/general/traverse_convo/test_traverse_convo.py b/convokit/tests/general/traverse_convo/test_traverse_convo.py index bc3a591f5..161e1fc59 100644 --- a/convokit/tests/general/traverse_convo/test_traverse_convo.py +++ b/convokit/tests/general/traverse_convo/test_traverse_convo.py @@ -5,6 +5,7 @@ construct_tree_corpus, construct_nonexistent_reply_to_corpus, construct_multiple_convo_id_corpus, + construct_no_timestamp_tree_corpus, ) from convokit.tests.test_utils import reload_corpus_in_db_mode @@ -66,6 +67,12 @@ def one_utt_convo(self): self.assertEqual([utt.id for utt in convo.traverse("postorder")], ["other"]) self.assertEqual([utt.id for utt in convo.traverse("preorder")], ["other"]) + def traverse_no_timestamps(self): + # sibling utterances without timestamps must not crash tree traversal + convo = self.no_timestamp_corpus.get_conversation("0") + bfs_traversal = [utt.id for utt in convo.traverse("bfs", as_utterance=True)] + self.assertEqual(bfs_traversal, ["0", "1", "2"]) + def reindex_corpus(self): original_convo_meta = { k: v for k, v in self.corpus.get_conversation("0").meta.to_dict().items() @@ -111,6 +118,9 @@ def setUp(self) -> None: self.nonexistent_reply_to_corpus = reload_corpus_in_db_mode( construct_nonexistent_reply_to_corpus() ) + self.no_timestamp_corpus = reload_corpus_in_db_mode( + construct_no_timestamp_tree_corpus() + ) def test_broken_convos(self): self.broken_convos() @@ -136,6 +146,9 @@ def test_conversation_id_to_leaf_paths(self): def test_one_utt_convo(self): self.one_utt_convo() + def test_traverse_no_timestamps(self): + self.traverse_no_timestamps() + def test_reindex_corpus(self): self.reindex_corpus() @@ -148,6 +161,7 @@ def setUp(self) -> None: self.corpus = construct_tree_corpus() self.multiple_convo_id_corpus = construct_multiple_convo_id_corpus() self.nonexistent_reply_to_corpus = construct_nonexistent_reply_to_corpus() + self.no_timestamp_corpus = construct_no_timestamp_tree_corpus() def test_broken_convos(self): self.broken_convos() @@ -173,6 +187,9 @@ def test_conversation_id_to_leaf_paths(self): def test_one_utt_convo(self): self.one_utt_convo() + def test_traverse_no_timestamps(self): + self.traverse_no_timestamps() + def test_reindex_corpus(self): self.reindex_corpus() diff --git a/convokit/tests/general/traverse_convo/traverse_convo_helpers.py b/convokit/tests/general/traverse_convo/traverse_convo_helpers.py index b141ba268..1da372c5b 100644 --- a/convokit/tests/general/traverse_convo/traverse_convo_helpers.py +++ b/convokit/tests/general/traverse_convo/traverse_convo_helpers.py @@ -82,6 +82,25 @@ def construct_nonexistent_reply_to_corpus(): return corpus +def construct_no_timestamp_tree_corpus(): + # a valid reply-to tree where the root has multiple children and no + # utterance has a timestamp set (timestamp is Optional and defaults to None) + corpus = Corpus( + utterances=[ + Utterance( + id="0", reply_to=None, conversation_id="0", speaker=Speaker(id="alice") + ), + Utterance( + id="1", reply_to="0", conversation_id="0", speaker=Speaker(id="alice") + ), + Utterance( + id="2", reply_to="0", conversation_id="0", speaker=Speaker(id="alice") + ), + ] + ) + return corpus + + def construct_tree_corpus(): corpus = Corpus( utterances=[