Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the "Fall" filter bug #752

Merged
merged 6 commits into from
Sep 19, 2023
Merged

Fixed the "Fall" filter bug #752

merged 6 commits into from
Sep 19, 2023

Conversation

MihaiSurdeanu
Copy link
Contributor

No description provided.

@@ -265,7 +265,7 @@ class NumericActions(seasonNormalizer: SeasonNormalizer, unitNormalizer: UnitNor
val (seasonMentions, otherMentions) = mentions.partition(m => m.foundBy.contains("season"))
val (springFall, otherSeasons) = seasonMentions.partition(m => m.text.equalsIgnoreCase("spring") || m.text.equalsIgnoreCase("fall"))
val trueSeasons = springFall.filter { m =>
m.tags.head.contains("NN") && {
m.tags.get.head.startsWith("NN") && {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

This encourages me to continue denoting Options by name (like tagsOpt) and wonder whether something like the TagSet class from Eidos should be ported. We probably shouldn't be using string operations for classifying tags.

Comment on lines 330 to 334
val sd = TempEvalFormatter.mkDate(seasonNorm.get.startDay, seasonNorm.get.startMonth, yearStart)
val ed = TempEvalFormatter.mkDate(seasonNorm.get.endDay, seasonNorm.get.endMonth, yearEnd)
val dm = DateRangeMention(m, sd, ed)

dm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somebody is going to sneak in here and give these variables real names ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

priority: ${ rulepriority }
type: token
pattern: |
[word=/^(1\d|2\d|3\d|4\d|5\d|6\d|7\d|8\d|9\d)$/]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this isn't [1-9]\d.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be. It started as just [1-2] and [8-9] then I incrementally expanded without optimizing. Please adjust!

Keith Alcock and others added 2 commits September 14, 2023 21:09
Regular expression and variable names in numeric routines
Clean up after my own complaints
@kwalcock kwalcock merged commit 1ca4472 into master Sep 19, 2023
@kwalcock kwalcock deleted the fall branch September 19, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants