-
Notifications
You must be signed in to change notification settings - Fork 104
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
Enhance nickname processing #122
base: master
Are you sure you want to change the base?
Conversation
aikimark
commented
Mar 22, 2021
- converted REGEXES from set() to list
- added optional label/tag to regex tuples
- created new self variable for nickname regexes
- changed nickname processing logic
- added tests for nicknames, including adding patterns and multiple nicknames
* changed regexes from set to list * added labels/tags as third item in tuples * changed processing of regexes input to TupleManager * added filtered list for nickname patterns * added/changed regexes
remove testREGEXES.py from repository
* test for adding nickname * test for multiple nicknames
* parse parenthesized suffixes (ret, vet) * converted unused suffix processing routine to preprocess the fullname ahead of nickname processing, since nickname patterns include a parenthesis-delimited pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments but I think I missed pressing the final submit button. Sorry it has taken me so long to get to reviewing this again. Generally it looks good, I just have a few questions and there's a few and I'm not sure about adding Justice as a title because it precludes it being a first name.
@@ -231,7 +231,7 @@ def __init__(self, | |||
self.first_name_titles = SetManager(first_name_titles) | |||
self.conjunctions = SetManager(conjunctions) | |||
self.capitalization_exceptions = TupleManager(capitalization_exceptions) | |||
self.regexes = TupleManager(regexes) | |||
self.regexes = TupleManager([tpl[:2] for tpl in REGEXES]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should use the local variable regexes
to preserve the ability to pass it as an attribute to a new instance (not that anyone is doing that). ([tpl[:2] for tpl in regexes])
.
What is the slice doing here? It's not clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a tag/label to some of the tuples. The slice returns the first two items in the tuples, omitting the tag/label data. The TupleManager object can still be used in the code. The regexes variable in the constants() is no longer a set object, just a list of tuples. I did this to preserve the order of the regex patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. But If someone tries to instantiate passing a TuperManger to regexes, it will have no effect because you are using the global variable instead of the local one. Need to replace REGEXES
for regexes
.
ex: name = HumanNam(regexes=myTupleManager)
would fail. (I guess I should have some tests for those instantiation attributes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. The list of compiled regex patterns for nicknames is different than the regexes that is fed into tuplemanager. I thought I'd left the tuplemanager-based regexes alone. I might have gotten a little confused by variables/functions with the same name. I'll take another look at it.
Some clarification would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a simple mistake of using the module constant instead of the attribute passed to the class' init function.
change:
self.regexes = TupleManager([tpl[:2] for tpl in REGEXES])
to
self.regexes = TupleManager([tpl[:2] for tpl in regexes])
Here's a test that should pass but will fail with your code above.
def test_custom_regex_constant(self):
t = {'test': 'test'}
c = Constants(regexes=t)
self.assertEqual(c.regexes, t)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I edited the last line of my test to fix the equals test)
@@ -243,7 +254,11 @@ def nickname(self): | |||
The person's nicknames. Any text found inside of quotes (``""``) or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description of what constitutes a nickname should probably be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We've exchanged thoughts on this via email.
I added a couple of new nickname patterns and 'standardized' the existing patterns. Do you still have those emails?
# full_name setter triggers the parse | ||
#======================================================== | ||
#IMPORTANT NOTE: | ||
# The followint statement must be the last one in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "The following statement...", also could combine with the existing comment:
The following statement must be the last line in _init__ because it triggers the parse using :py:func:`full_name.setter`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. These two statements can be combined. I was 'bitten' by that when I started to change the code. I wanted to add text to really draw the attention of future collaborators.
@@ -6,6 +6,7 @@ | |||
'esq', | |||
'esquire', | |||
'jr', | |||
'jr.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of these suffixes are abbreviations that could also end in a period, eg "esq". Some of them are not abbreviations, eg "iii". Do we need to enter period versions of all of them that are abbreviations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to verify how you are processing the suffixes to respond with 100% certainty. Since the suffixes are in a list, I think the code is most likely doing a set inclusion test, using the in operator. Part of the answer depends on how you are parsing the text prior to doing the set inclusion test. If your parsing picks up periods, then my answer is "yes" - you will need to have both the plain and period-ending versions of the suffixes. Also, be aware that the set inclusion test is case sensitive.
In the new suffix preprocessing routine I added, the regex pattern has a ".?", which will match the prior text with and without a trailing period character. The question mark makes the period character optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to happen?
The parsing code does a split on space characters, so any trailing punctuation (comma, period, semicolon, etc.) will remain with the word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the utility function lc()
removes periods.
Looking at the is_suffix
method in the parser,
# suffixes may have periods inside them like "M.D."
return ((lc(piece).replace('.','') in self.C.suffix_acronyms) \
or (lc(piece) in self.C.suffix_not_acronyms)) \
and not self.is_an_initial(piece)
Looks like it was already a bit unclear before you started because .replace('.','')
is unnecessary in that first line.
So, I think you should remove the "jr." entry because I don't expect it is making anything work. Probably there's something not quite right there but it's not related to any changes you made and we can fix it after merging your changes in if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is period removal sufficient? I'm thinking about other punctuation characters that may separate suffixes, such as comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first step of the parser is to split the entire string on commas. Many suffixes listed at the end of a name are always treated as suffixes. These are the 2 supported formats for commas:
Lastname [Suffix], Title Firstname (Nickname) Middle Middle[,] Suffix [, Suffix]
Title Firstname M Lastname [Suffix], Suffix [Suffix] [, Suffix]
@@ -25,6 +26,7 @@ | |||
""" | |||
SUFFIX_ACRONYMS = set([ | |||
'(ret)', | |||
'(ret.)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SUFFIX_ACRONYMS set was intended to be acronyms that could optionally be separated by periods. I'm not sure how (ret) got in there because it's surprising to see parenthesis in there, but it doesn't seem to make sense to add a period to one of the items in this set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "(ret)" was there when I started coding. I didn't know how you might want to parse the "ret" and "vet" suffixes, so I tried to keep the parentheses in the parsed result. I think I added trailing-period versions to the list before I realized that any parenthesis-delimited string would be parsed by the nickname routine. I can certainly change how I'm processing these two as well as add other suffix strings to the preprocessing pattern. Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to happen with ret and vet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does something fail if you remove the one with the period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I'm using a regex pattern for the (ret) and (vet) suffixes. The regex pattern ignores any trailing period. Using the existing list of suffixes doesn't work, since the nickname processing picks these up, matching the parenthesis pattern. Nickname processing happens before suffix processing, which is why I added a suffix preprocessor routine for these two parenthesis-delimited suffixes.
I can retain the parentheses or drop them. Let me know and I'll make sure the code does what you want, relative to these two suffixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have changed the way that suffixes are detected then I think you need to modify the is_suffix
method to implement that test using your new regexes.
@@ -166,6 +166,7 @@ | |||
'chef', | |||
'chemist', | |||
'chief', | |||
'chief justice', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised if this works, but I guess I could see it because of how the titles chain together. "Justice" is a somewhat common first name so we couldn't just add that as it's own title, so if this works it's a nice workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that "Justice" was a common first name. I don't think my "Chief Justice" string is being matched due to the prior parsing actions. I'm not sure what qualifies as "common first name". "Justice" is around the 580th most common first name in America. However, I think it is probably more common than the number of judges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke with a judge and asked him about the use of the title "Justice". He said it was rare. I'll undo this change, since it was based on a false assumption.
The judge expressed some dismay that titles were being used as first name. He has encountered people with first names, such as "King" and "Queen", in his courtroom.
We might want to include the parser's bias for first names over titles.
If someone is parsing names of titled people (think UN delegations), what should they do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bias for first names over titles is already a feature of the parser, and why there are no potential first names in the titles constant. First job of a name parser is to parse names, then it can optionally parse titles but not if it messes up names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do some testing after removing "Justice" and "Chief Justice" from the list. I might add a tests for "David Justice" and "Justice, David", the baseball player.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I work in legal data, and I'll note that there are very few "Justices" though I suppose "justice of the peace" is a title. But in my experience, "justice" is reserved pretty much exclusively for the SCOTUS justices. You can see the way this shakes out on this page (though it doesn't discuss this topic): https://www.uscourts.gov/judges-judgeships/about-federal-judges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, meant to say that relatedly, "J." is a very common title among judges. I'm guessing it can't be added b/c it's one letter, but I thought I'd throw that out there.
tests.py
Outdated
@@ -1766,6 +1796,21 @@ def test_suffix_with_periods_with_lastname_comma(self): | |||
self.m(hn.last, "Doe", hn) | |||
self.m(hn.suffix, "Msc.Ed.", hn) | |||
|
|||
@unittest.SkipTest | |||
def test_suffix_in_nickname_dup(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to have comments about what these skipped tests are doing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at this. I might have added these tests for my initial approach to the problem. Once I changed my approach, I just skipped the test rather than delete the (test_) function. Looking at the code will jog my memory.
I'm going to ask a retired judge about the titles that one might apply to his profession. I'll email you after we've talked.
@@ -339,6 +340,7 @@ | |||
'judicial', | |||
'junior', | |||
'jurist', | |||
'justice', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My friend Justice would be upset that the parser would not recognize his first name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are about the same number of people named "Justice" as there are judges in America (~33000). What does the name parser do, or what should it do, when it encounters several names? What if "Justice" is one of the first of the words in a multi-name string?
This is a question similar to the one that I posed for myself when I first approached the problem of nicknames that might also be suffixes. I didn't have a good answer, so I abandoned that original approach. It is still an unanswered question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplest use case for this parser is just Firstname Lastname. I feel like when there is conflict with other things the parser should/could do (ex: recognize titles), those other things should be sacrificed to preserve it's ability to split up a simple name. There is a fairly simple workaround if someone using the parser wants to change it, and a human interacting with the parser could add their fist name and the parser would then figure out that it's a title, kind of like if you were interacting with a human and they had the same confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed a test for this. I'll look at it and alter my John Roberts test accordingly.
I'll remove "Justice" from the titles list.
I have responded via email. Should I copy my response into the textboxes above? This is my first pull request. |
I think if you respond in the github UI it just facilitates the conversations about comment. I will take a closer look at your email later tonight.
- Derek Gulbranson
… On May 4, 2021, at 6:37 PM, aikimark ***@***.***> wrote:
I have responded via email.
Should I copy my response into the textboxes above? This is my first pull request.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you have to make any changes to this pull request in your clone of the project and then push them to this change request somehow.
@@ -231,7 +231,7 @@ def __init__(self, | |||
self.first_name_titles = SetManager(first_name_titles) | |||
self.conjunctions = SetManager(conjunctions) | |||
self.capitalization_exceptions = TupleManager(capitalization_exceptions) | |||
self.regexes = TupleManager(regexes) | |||
self.regexes = TupleManager([tpl[:2] for tpl in REGEXES]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. But If someone tries to instantiate passing a TuperManger to regexes, it will have no effect because you are using the global variable instead of the local one. Need to replace REGEXES
for regexes
.
ex: name = HumanNam(regexes=myTupleManager)
would fail. (I guess I should have some tests for those instantiation attributes.)