Repository: nltk/nltk
Pull request: Prevent `debug_decisions` in Punkt from throwing IndexError
Fixes #2519

---

Hello!

### Pull request overview
* Prevent `debug_decisions` in Punkt from throwing IndexError whenever the passed sentence contained a sentence ending in `?`, `!` or a custom defined end character (under `_lang_vars.sent_end_chars`). 
* Added 4 tests to show that this no longer throws an Exception.
* Modified `nltk/test/unit/test_tokenize.py` to only skip relevant tests if Stanford Segmenter/Tokenizer jars are missing, instead of skipping all tokenizer tests.
* Modernised the code slightly, e.g. update a format string to the `.format()` style.

---

### The bug & the fix
The `IndexError` is thrown in the following two lines, where `tokens` is a list of `PunktToken` instances:
https://github.com/nltk/nltk/blob/1244ab9fc4b130ae20127267d49eea506e30a9ae/nltk/tokenize/punkt.py#L1288-L1289

If no element in this list ends with a period, then the while loop will only terminate through an `IndexError` when `tokens` becomes empty. 
The loop here aims to remove all arbitrary words that happen before the sentence break. This way, only the word before and after the sentence break are shown. However, a sentence break may also occur on a `?`, or a `!`. In these cases, the loop will throw an Exception.

So, instead of discarding until there's a token that ends with a period, it now discards until there's a token that ends with any sent_end_char (by default: `(".", "?", "!")`).
This has removed the issue. However, just to be safe, I also added `while tokens and tokens[0]...`. This way the loop can never throw an `IndexError`.

The bug can be reproduced by running any of the new tests on the old codebase.

---

### Additional changes
I noticed that `nltk/test/unit/test_tokenize.py` does not run any tests whatsoever if the Stanford segmenter/tokenizer jars are not installed. The following function is responsible for this:
https://github.com/nltk/nltk/blob/1244ab9fc4b130ae20127267d49eea506e30a9ae/nltk/test/unit/test_tokenize.py#L17-L34

(Furthermore, `StanfordTokenizer` is never imported?)

I've modified this slightly to create a `pytest.mark.skipif` object (See [here](https://docs.pytest.org/en/latest/how-to/skipping.html#id1) for documentation) which is used on the two methods that test the Stanford Segmenter. The `try-except` around these two methods were also removed as they are no longer needed. Mentions of the Stanford Tokenizer are removed, as they weren't actually tested. 

The main goal of these changes is to keep testing the other tokenizers (tweet, punkt, etc.) even if the Stanford files are not installed.
@dannysepler I know this is your domain, so I'm asking for you to have a quick look at this. 

I also updated some small things, like moving to `.format()` formatting, 

- Tom Aarsen

Problem statement:
PunktSentenceTokenizer doesn't work with custom end sentence character
Example:

```
import nltk
from nltk.tokenize.punkt import PunktLanguageVars

class ExtLangVars(PunktLanguageVars):
    sent_end_chars = ('.', '?', '!', '^')

punkt_tk = nltk.data.load('tokenizers/punkt/english.pickle')
punkt_tk._lang_vars = ExtLangVars()

txt1 = 'Subject: Some subject. Attachments: Some attachemnts'
txt2 = 'Subject: Some subject^ Attachments: Some attachemnts'

r = punkt_tk.tokenize(txt1)
print(r)

r = punkt_tk.tokenize(txt2)
print(r)

d = punkt_tk.debug_decisions(txt1)
for x in d:
    print(nltk.tokenize.punkt.format_debug_decision(x))

#d = punkt_tk.debug_decisions(txt2)
#for x in d:
#    print(nltk.tokenize.punkt.format_debug_decision(x))

```
Also if you uncomment last lines, it will fail with index out of bounds. I think it's because of hardcoded 

`        self.period_final = tok.endswith('.')`

in PunktToken. Tried many ways to work around this, but no luck. Any suggestions would be appreciated.

Hints from issue discussion:

Commit message: Renamed `stanford_segmenter` to `check_stanford_segmenter`

Also trimmed whitespace

Patch to review:
diff --git a/nltk/test/unit/test_tokenize.py b/nltk/test/unit/test_tokenize.py
index 79110e3b4c..a9927cedd0 100644
--- a/nltk/test/unit/test_tokenize.py
+++ b/nltk/test/unit/test_tokenize.py
@@ -14,24 +14,20 @@
     LegalitySyllableTokenizer,
 )
 
-def setup_module(module):
-    import pytest
 
+def load_stanford_segmenter():
     try:
         seg = StanfordSegmenter()
         seg.default_config("ar")
         seg.default_config("zh")
-    except LookupError as e:
-        pytest.skip(
-            "Tests for nltk.tokenize.stanford_segmenter skipped: %s" % str(e)
-        )
-
-    try:
-        StanfordTokenizer()
+        return True
     except LookupError:
-        pytest.skip(
-            "Tests for nltk.tokenize.stanford are skipped because the stanford postagger jar doesn't exist"
-        )
+        return False
+
+
+check_stanford_segmenter = pytest.mark.skipif(
+    not load_stanford_segmenter(), reason="NLTK was unable to find stanford-segmenter.jar."
+)
 
 
 class TestTokenize:
@@ -75,44 +71,40 @@ def test_legality_principle_syllable_tokenizer(self):
         tokens = tokenizer.tokenize(test_word)
         assert tokens == ['won', 'der', 'ful']
 
+    @check_stanford_segmenter
     def test_stanford_segmenter_arabic(self):
         """
         Test the Stanford Word Segmenter for Arabic (default config)
         """
-        try:
-            seg = StanfordSegmenter()
-            seg.default_config('ar')
-            sent = u'يبحث علم الحاسوب استخدام الحوسبة بجميع اشكالها لحل المشكلات'
-            segmented_sent = seg.segment(sent.split())
-            assert segmented_sent.split() == [
-                'يبحث',
-                'علم',
-                'الحاسوب',
-                'استخدام',
-                'الحوسبة',
-                'ب',
-                'جميع',
-                'اشكال',
-                'ها',
-                'ل',
-                'حل',
-                'المشكلات',
-            ]
-        except LookupError as e:
-            pytest.skip(str(e))
+        seg = StanfordSegmenter()
+        seg.default_config('ar')
+        sent = u'يبحث علم الحاسوب استخدام الحوسبة بجميع اشكالها لحل المشكلات'
+        segmented_sent = seg.segment(sent.split())
+        assert segmented_sent.split() == [
+            'يبحث',
+            'علم',
+            'الحاسوب',
+            'استخدام',
+            'الحوسبة',
+            'ب',
+            'جميع',
+            'اشكال',
+            'ها',
+            'ل',
+            'حل',
+            'المشكلات',
+        ]
 
+    @check_stanford_segmenter
     def test_stanford_segmenter_chinese(self):
         """
         Test the Stanford Word Segmenter for Chinese (default config)
         """
-        try:
-            seg = StanfordSegmenter()
-            seg.default_config('zh')
-            sent = u"这是斯坦福中文分词器测试"
-            segmented_sent = seg.segment(sent.split())
-            assert segmented_sent.split() == ['这', '是', '斯坦福', '中文', '分词器', '测试']
-        except LookupError as e:
-            pytest.skip(str(e))
+        seg = StanfordSegmenter()
+        seg.default_config('zh')
+        sent = u"这是斯坦福中文分词器测试"
+        segmented_sent = seg.segment(sent.split())
+        assert segmented_sent.split() == ['这', '是', '斯坦福', '中文', '分词器', '测试']
 
     def test_phone_tokenizer(self):
         """
@@ -470,3 +462,51 @@ def test_punkt_tokenize_no_custom_lang_vars(self):
         expected = ["উপরাষ্ট্রপতি শ্রী এম ভেঙ্কাইয়া নাইডু সোমবার আই আই টি দিল্লির হীরক জয়ন্তী উদযাপনের উদ্বোধন করেছেন। অনলাইনের মাধ্যমে এই অনুষ্ঠানে কেন্দ্রীয় মানব সম্পদ উন্নয়নমন্ত্রী শ্রী রমেশ পোখরিয়াল ‘নিশাঙ্ক’  উপস্থিত ছিলেন। এই উপলক্ষ্যে উপরাষ্ট্রপতি হীরকজয়ন্তীর লোগো এবং ২০৩০-এর জন্য প্রতিষ্ঠানের লক্ষ্য ও পরিকল্পনার নথি প্রকাশ করেছেন।"]
 
         assert obj.tokenize(sentences) == expected
+
+    def test_punkt_debug_decisions_dot(self):
+        # Test debug_decisions on a text with two sentences, split by a dot.
+        tokenizer = punkt.PunktSentenceTokenizer()
+        txt = "Subject: Some subject. Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_exclamation_mark(self):
+        # Test debug_decisions on a text with two sentences, split by a dot.
+        tokenizer = punkt.PunktSentenceTokenizer()
+        txt = "Subject: Some subject! Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_custom_end(self):
+        # Test debug_decisions on a text with two sentences,
+        # split by a custom end character, based on Issue #2519
+        tokenizer = punkt.PunktSentenceTokenizer()
+
+        class ExtLangVars(punkt.PunktLanguageVars):
+            sent_end_chars = ('.', '?', '!', '^')
+
+        tokenizer._lang_vars = ExtLangVars()
+
+        txt = "Subject: Some subject^ Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_no_split(self):
+        # Test debug_decisions on a text with one sentences,
+        # which is not split.
+        tokenizer = punkt.PunktSentenceTokenizer()
+
+        txt = "This is just a normal sentence, just like any other."
+
+        # Test that the sentence is not split.
+        assert len(tokenizer.tokenize(txt)) == 1
+        assert len(list(tokenizer.debug_decisions(txt))) == 0
diff --git a/nltk/tokenize/punkt.py b/nltk/tokenize/punkt.py
index 512f6630ec..9f04be468c 100644
--- a/nltk/tokenize/punkt.py
+++ b/nltk/tokenize/punkt.py
@@ -1285,27 +1285,27 @@ def debug_decisions(self, text):
             decision_text = match.group() + match.group("after_tok")
             tokens = self._tokenize_words(decision_text)
             tokens = list(self._annotate_first_pass(tokens))
-            while not tokens[0].period_final:
+            while tokens and not tokens[0].tok.endswith(self._lang_vars.sent_end_chars):
                 tokens.pop(0)
-            yield dict(
-                period_index=match.end() - 1,
-                text=decision_text,
-                type1=tokens[0].type,
-                type2=tokens[1].type,
-                type1_in_abbrs=bool(tokens[0].abbr),
-                type1_is_initial=bool(tokens[0].is_initial),
-                type2_is_sent_starter=tokens[1].type_no_sentperiod
-                in self._params.sent_starters,
-                type2_ortho_heuristic=self._ortho_heuristic(tokens[1]),
-                type2_ortho_contexts=set(
+            yield {
+                "period_index": match.end() - 1,
+                "text": decision_text,
+                "type1": tokens[0].type,
+                "type2": tokens[1].type,
+                "type1_in_abbrs": bool(tokens[0].abbr),
+                "type1_is_initial": bool(tokens[0].is_initial),
+                "type2_is_sent_starter": tokens[1].type_no_sentperiod 
+                                         in self._params.sent_starters,
+                "type2_ortho_heuristic": self._ortho_heuristic(tokens[1]),
+                "type2_ortho_contexts": set(
                     self._params._debug_ortho_context(tokens[1].type_no_sentperiod)
                 ),
-                collocation=(tokens[0].type_no_sentperiod, tokens[1].type_no_sentperiod)
-                in self._params.collocations,
-                reason=self._second_pass_annotation(tokens[0], tokens[1])
-                or REASON_DEFAULT_DECISION,
-                break_decision=tokens[0].sentbreak,
-            )
+                "collocation": (tokens[0].type_no_sentperiod, tokens[1].type_no_sentperiod)
+                               in self._params.collocations,
+                "reason": self._second_pass_annotation(tokens[0], tokens[1])
+                          or REASON_DEFAULT_DECISION,
+                "break_decision": tokens[0].sentbreak,
+            }
 
     def span_tokenize(self, text, realign_boundaries=True):
         """
@@ -1633,21 +1633,21 @@ def _ortho_heuristic(self, aug_tok):
         return "unknown"
 
 
-DEBUG_DECISION_FMT = """Text: %(text)r (at offset %(period_index)d)
-Sentence break? %(break_decision)s (%(reason)s)
-Collocation? %(collocation)s
-%(type1)r:
-    known abbreviation: %(type1_in_abbrs)s
-    is initial: %(type1_is_initial)s
-%(type2)r:
-    known sentence starter: %(type2_is_sent_starter)s
-    orthographic heuristic suggests is a sentence starter? %(type2_ortho_heuristic)s
-    orthographic contexts in training: %(type2_ortho_contexts)s
+DEBUG_DECISION_FMT = """Text: {text!r} (at offset {period_index})
+Sentence break? {break_decision} ({reason})
+Collocation? {collocation}
+{type1!r}:
+    known abbreviation: {type1_in_abbrs}
+    is initial: {type1_is_initial}
+{type2!r}:
+    known sentence starter: {type2_is_sent_starter}
+    orthographic heuristic suggests is a sentence starter? {type2_ortho_heuristic}
+    orthographic contexts in training: {type2_ortho_contexts}
 """
 
 
 def format_debug_decision(d):
-    return DEBUG_DECISION_FMT % d
+    return DEBUG_DECISION_FMT.format(**d)
 
 
 def demo(text, tok_cls=PunktSentenceTokenizer, train_cls=PunktTrainer):


Code review comments:
diff_hunk: @@ -470,3 +462,51 @@ def test_punkt_tokenize_no_custom_lang_vars(self):
         expected = ["উপরাষ্ট্রপতি শ্রী এম ভেঙ্কাইয়া নাইডু সোমবার আই আই টি দিল্লির হীরক জয়ন্তী উদযাপনের উদ্বোধন করেছেন। অনলাইনের মাধ্যমে এই অনুষ্ঠানে কেন্দ্রীয় মানব সম্পদ উন্নয়নমন্ত্রী শ্রী রমেশ পোখরিয়াল ‘নিশাঙ্ক’  উপস্থিত ছিলেন। এই উপলক্ষ্যে উপরাষ্ট্রপতি হীরকজয়ন্তীর লোগো এবং ২০৩০-এর জন্য প্রতিষ্ঠানের লক্ষ্য ও পরিকল্পনার নথি প্রকাশ করেছেন।"]
 
         assert obj.tokenize(sentences) == expected
+
+    def test_punkt_debug_decisions_dot(self):
+        # Test debug_decisions on a text with two sentences, split by a dot.
+        tokenizer = punkt.PunktSentenceTokenizer()
+        txt = "Subject: Some subject. Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_exclamation_mark(self):
+        # Test debug_decisions on a text with two sentences, split by a dot.
+        tokenizer = punkt.PunktSentenceTokenizer()
+        txt = "Subject: Some subject! Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_custom_end(self):
+        # Test debug_decisions on a text with two sentences,
+        # split by a custom end character, based on Issue #2519
+        tokenizer = punkt.PunktSentenceTokenizer()
+
+        class ExtLangVars(punkt.PunktLanguageVars):
+            sent_end_chars = ('.', '?', '!', '^')
+
+        tokenizer._lang_vars = ExtLangVars()
+
+        txt = "Subject: Some subject^ Attachments: Some attachemnts"
+
+        # Test that the sentence is split into two sections,
+        # with one split and hence one decision.
+        assert len(tokenizer.tokenize(txt)) == 2
+        assert len(list(tokenizer.debug_decisions(txt))) == 1
+
+    def test_punkt_debug_decisions_no_split(self):
path: nltk/test/unit/test_tokenize.py
text: @user1:
optional: you could do `test_punkt_debug_decisions_no_split`, `test_punkt_debug_decisions_exclamation_mark`, and `test_punkt_debug_decisions_dot` as one test with `pytest.mark.parametrize`

@author:
I've done so! Thank you for the reminder and recommendation.

diff_hunk: @@ -470,3 +462,51 @@ def test_punkt_tokenize_no_custom_lang_vars(self):
         expected = ["উপরাষ্ট্রপতি শ্রী এম ভেঙ্কাইয়া নাইডু সোমবার আই আই টি দিল্লির হীরক জয়ন্তী উদযাপনের উদ্বোধন করেছেন। অনলাইনের মাধ্যমে এই অনুষ্ঠানে কেন্দ্রীয় মানব সম্পদ উন্নয়নমন্ত্রী শ্রী রমেশ পোখরিয়াল ‘নিশাঙ্ক’  উপস্থিত ছিলেন। এই উপলক্ষ্যে উপরাষ্ট্রপতি হীরকজয়ন্তীর লোগো এবং ২০৩০-এর জন্য প্রতিষ্ঠানের লক্ষ্য ও পরিকল্পনার নথি প্রকাশ করেছেন।"]
 
         assert obj.tokenize(sentences) == expected
+
+    def test_punkt_debug_decisions_dot(self):
+        # Test debug_decisions on a text with two sentences, split by a dot.
+        tokenizer = punkt.PunktSentenceTokenizer()
+        txt = "Subject: Some subject. Attachments: Some attachemnts"
path: nltk/test/unit/test_tokenize.py
text: @user1:
spelling of `attachemnts`

@author:
I've resolved this. The typo was already present in the example I took it from, but I suppose that the issue (obviously) still persists with the non-typo version. So, it's probably preferable to fix the typo, rather than stay true to the original example.