Agreement by thorunna · Pull Request #61 · mideind/GreynirCorrect

thorunna · 2023-10-13T10:58:04Z

Handling for agreement errors added.

vthorsteinsson · 2023-10-13T17:35:48Z

src/reynir_correct/pattern.py

+        sbj = match.first_match("NP-SUBJ")
+        if sbj is None:
+            return
+        if len(sbj) > 1:  # TODO: more accurate subject selection


Can this ever happen, with first_match()?

This check is used in similar cases multiple times in the script, would have to check further if it's necessary

vthorsteinsson · 2023-10-13T17:36:45Z

src/reynir_correct/pattern.py

        )

+    def agreement_conj(self, match: SimpleTree) -> None:
+        vp = match.first_match("VP > (so_ft|so_et)")


Would be nice to have a comment here that describes exactly what this rule is correcting, preferably with an example.

vthorsteinsson · 2023-10-13T17:36:59Z

src/reynir_correct/pattern.py

+        )
+
+    def agreement_subpost_sing(self, match: SimpleTree) -> None:
+        vp = match.first_match("VP > (so_ft)")


Again, a comment would be useful.

Are you using black for formatting? That line is 153 characters wide! :)

I'm using black but for some reason it's not working on a part of this script. Fixed the wide lines I saw :)

vthorsteinsson · 2023-10-13T17:37:29Z

src/reynir_correct/pattern.py

+        sbj = match.first_match("NP-SUBJ > { (no_nf_et|fn_nf_et) }")
+        if sbj is None:
+            return
+        if len(sbj) > 1:  # TODO: more accurate subject selection


Can this ever happen, with first_match()?

Again, would have to look further into this

vthorsteinsson · 2023-10-13T17:37:50Z

src/reynir_correct/pattern.py

+        )
+
+    def agreement_concord(self, match: SimpleTree) -> None:
+        np = match.first_match("NP")


Add comment?

vthorsteinsson · 2023-10-13T17:38:24Z

src/reynir_correct/pattern.py

+    def agreement_concord(self, match: SimpleTree) -> None:
+        np = match.first_match("NP")
+        # if vp is None: return
+        assert np is not None


Might be more robust to do an if...return, instead of assert().

vthorsteinsson · 2023-10-13T17:39:05Z

src/reynir_correct/pattern.py

+
+        self.add_pattern(
+            (
+                "þessi",  # Trigger lemma for this pattern


Hvað með "sá"?

This pattern doesn't make sense since it's triggered by 'þessi' in a possessive noun phrase, which probably wouldn't happen. It then calls agreement_conj() which relates to conjuctions like the ones here above. Maybe it would be best to delete this pattern.

thorunna · 2023-10-23T15:52:05Z

src/reynir_correct/pattern.py

        )

+    def agreement_conj(self, match: SimpleTree) -> None:
+        vp = match.first_match("VP > (so_ft|so_et)")


thorunna · 2023-10-23T15:54:50Z

src/reynir_correct/pattern.py

+        sbj = match.first_match("NP-SUBJ")
+        if sbj is None:
+            return
+        if len(sbj) > 1:  # TODO: more accurate subject selection


This check is used in similar cases multiple times in the script, would have to check further if it's necessary

thorunna · 2023-10-23T15:58:20Z

src/reynir_correct/pattern.py

+        )
+
+    def agreement_subpost_sing(self, match: SimpleTree) -> None:
+        vp = match.first_match("VP > (so_ft)")


thorunna · 2023-10-23T15:58:49Z

src/reynir_correct/pattern.py

+        sbj = match.first_match("NP-SUBJ > { (no_nf_et|fn_nf_et) }")
+        if sbj is None:
+            return
+        if len(sbj) > 1:  # TODO: more accurate subject selection


Again, would have to look further into this

thorunna · 2023-10-23T16:01:10Z

src/reynir_correct/pattern.py

+        )
+
+    def agreement_concord(self, match: SimpleTree) -> None:
+        np = match.first_match("NP")


thorunna · 2023-10-25T09:32:10Z

src/reynir_correct/pattern.py

+    def agreement_concord(self, match: SimpleTree) -> None:
+        np = match.first_match("NP")
+        # if vp is None: return
+        assert np is not None


thorunna · 2023-10-25T09:40:22Z

src/reynir_correct/pattern.py

+
+        self.add_pattern(
+            (
+                "þessi",  # Trigger lemma for this pattern


This pattern doesn't make sense since it's triggered by 'þessi' in a possessive noun phrase, which probably wouldn't happen. It then calls agreement_conj() which relates to conjuctions like the ones here above. Maybe it would be best to delete this pattern.

thorunna · 2023-10-25T09:41:16Z

src/reynir_correct/pattern.py

+            )
+        )
+
+    def agreement_subpost_sing(self, match: SimpleTree) -> None:


No pattern in the script triggers this function. Should I delete it or keep it in case a pattern will be added?

thorunna · 2023-10-25T09:41:38Z

src/reynir_correct/pattern.py

+            )
+        )
+
+    def agreement_concord(self, match: SimpleTree) -> None:


Again, no pattern in the script triggers this function. Should I delete it or keep it in case a pattern will be added?

…to agreement

thorunna added 7 commits August 13, 2021 15:18

first handling for agreement-conj added

d6e7e7c

agreement-conj added

ac4e248

added handling for agreement errors

8564902

more handling for agreement-conj

32c2ab4

handling for agreement-subpost added

556a7d8

handling for agreement-concord added

6bd5dfc

merge master into branch

d045890

vthorsteinsson reviewed Oct 13, 2023

View reviewed changes

responded to comments

c61c137

thorunna commented Oct 25, 2023

View reviewed changes

thorunna added 2 commits October 26, 2023 09:34

responded to comment

287cdac

Merge branch 'master' of https://github.com/mideind/GreynirCorrect in…

435678d

…to agreement

Conversation

thorunna commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants