Skip to content

Implement from_segments #32

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.

Sign up for GitHub

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

Open
arubehn wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement from_segments #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c281e3
test
arubehn Jul 17, 2024
44dc9f3
test complete
arubehn Jul 17, 2024
fa11cde
implement parsing segmented sequences
arubehn Jul 17, 2024
93485da
implement Word.reversed_segments
arubehn Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions src/linse/typedsequence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,26 @@ class Morpheme(TypedSequence): # noqa: N801
item_separator = ' '

@classmethod
def from_string(cls, s):
if re.search(r'\s+', s):
# We assume that s is a whitespace-separated list of segments:
s = s.split()
def from_string(cls, s, separator=None):
separator = separator or cls.item_separator
if separator.strip(): # if the separator is something else than whitespaces in any form
separator = "\s*" + re.escape(separator) + "\s*"
s = re.split(separator, s)
else:
#
# FIXME: do segmentation here!
#
s = list(s)
if re.search(r'\s+', s):
# We assume that s is a whitespace-separated list of segments:
s = s.split()
else:
#
# FIXME: do segmentation here!
#
s = list(s)
return cls(s)

@classmethod
def from_segments(cls, s, separator=None):
return s.split(separator) if separator else s.split()
Copy link
Contributor

@xrotwang xrotwang Jul 17, 2024

Choose a reason for hiding this comment

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

I find this more obfuscation than convenience. Isn't

cls.from_segments(s.split('_'))

more explicit than

cls.from_segments(s, separator='_')

? I.e. if the caller already knows what to split on, then they should do it right away.

Copy link
Contributor

@LinguList LinguList Jul 18, 2024

Choose a reason for hiding this comment

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

One aspect that I would say should not be forgotten here is that we would have two situations that happen a lot in calling the function:

  1. There is a string, so we want to split by the separator. Normally, actually ALWAYS, it is a + by which we split, so we have cls.from_segments(string) as the convenient normal case. If we have to use s.split(" + ") here also always, we end up writing many more lines.
  2. There is a list, if we read in data from cldf, where the split is done on due to the way we handle the data as a list there.

Copy link
Contributor

@LinguList LinguList Jul 18, 2024

Choose a reason for hiding this comment

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

So if we want to say the from_segments is dealing with Segments in Lingpy aka TOKENS and CLDF aka CLDF_Segments, I'd consider it advantageous to have a check if it is a list and then revert it. But I know this is may obfuscate it even more.

But the handling with separator as kw is something I consider an urgent convenience, since we have the default here, which we'd otherwise have to invoke ALWAYS via s.split(" + ").

Copy link
Contributor

@xrotwang xrotwang Jul 18, 2024

Choose a reason for hiding this comment

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

Hm. Maybe we should have even more factory methods? I want to avoid the "seems to work" situations, i.e. situations where you are not forced to think about what your input actually looks like - yet something seems to happen and you just accept the results. Having separate methods that only accept one datatype as input force you to think about this - and allow tools like PyCharm to help you with this.

Copy link
Contributor

@xrotwang xrotwang Jul 18, 2024

Choose a reason for hiding this comment

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

Another advantage of additional methods is that methods have docstrings, so we get a canonical place where to document the clever things we might do to manipulate input :)

Copy link
Contributor

@LinguList LinguList Jul 21, 2024

Choose a reason for hiding this comment

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

    @classmethod
    def from_segments(cls, s):
        """
        only accepts list!
        """

Is different from __init__.

Here, we have a list like ["p", "a", "+", "t", "e", "r"]. But we want internally [["p", "a"], ["t", "e", "r"]].

Copy link
Contributor

@LinguList LinguList Jul 21, 2024

Choose a reason for hiding this comment

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

One way to address this is " ".join(["p", "a", "+", "t", "e", "r"]).split(" + "), but I guess I would prefer a direct solution by iterating over the list and then splitting.

import itertools # using groupby

split_by = lambda lst: [list(group) for k, group in itertools.groupby(lst, lambda x: x == "+") if not k]

Example:

>>> split_by("p a t + e r + e r".split())
[['p', 'a', 't'], ['e', 'r'], ['e', 'r']]

Copy link
Contributor

@LinguList LinguList Jul 21, 2024

Choose a reason for hiding this comment

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

I would not use lambda, it was to show how this works. I got the solution after checking again on itertools, looking for the opposit of itertools.chain and then I found this blog demonstrating the solution.

Copy link
Author

@arubehn arubehn Jul 22, 2024

Choose a reason for hiding this comment

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

@LinguList you are right, thank you for pointing that out. I have not thought about this case - then, it does seem reasonable to me to have three factory methods (all of which require some sort of preprocessing before calling __init__ and allow for a custom separator), as you guys have suggested. I can quickly implement that :)

Copy link
Author

@arubehn arubehn Jul 22, 2024

Choose a reason for hiding this comment

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

And I think itertools.groupby is better than using a hand-forged solution.


def to_text(self):
return ''.join(self)

Expand All @@ -160,26 +169,38 @@ class Word(TypedSequence):
item_separator = ' + '

@classmethod
def from_string(cls, s: str, **kw):
def from_string(cls, s: str, separator=None, **kw):
separator = separator or cls.item_separator
kw['type'] = Morpheme
# We assume s is a list of morphemes separated by +:
return cls(iterable=[
Morpheme.from_string(m.strip()) for m in s.split(cls.item_separator.strip())], **kw)
Morpheme.from_string(m.strip()) for m in s.split(separator.strip())], **kw)

@classmethod
def from_segments(cls, s, separator=None, **kw):
separator = separator or cls.item_separator
pattern = r"\s+" + re.escape(separator.strip()) + r"\s+"
return cls(iterable=[
Morpheme.from_segments(m) for m in re.split(pattern, s)], **kw)

def to_text(self):
return ''.join(m.to_text() for m in self)

def reversed_segments(self):
return Word([m[::-1] for m in self[::-1]])

Copy link
Contributor

@xrotwang xrotwang Aug 8, 2024

Choose a reason for hiding this comment

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

Looks good to me.

What do we do with the other changes in this PR?

Copy link
Author

@arubehn arubehn Aug 8, 2024

Choose a reason for hiding this comment

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

That's up to you to decide, of course ;) but since I didn't touch any of the existing methods, I think it would be no harm to keep the from_segments method around -- or do you suggest doing something differently?


class Phrase(TypedSequence):
item_type = Word
item_separator = ' _ '

@classmethod
def from_string(cls, s: str, **kw): # pragma: no cover
def from_string(cls, s: str, separator=None, **kw): # pragma: no cover
separator = separator or cls.item_separator
kw['type'] = Word
# We assume s is a list of morphemes separated by +:
return cls(iterable=[
Word.from_string(m.strip()) for m in s.split(cls.item_separator.strip())], **kw)
Word.from_string(m.strip()) for m in s.split(separator.strip())], **kw)

@classmethod
def from_text(cls, text):
Expand Down
10 changes: 10 additions & 0 deletions tests/test_typedsequence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,16 @@ def test_Word():
s[0] = Morpheme("b c d".split())
assert str(s[0]) == "b c d"

# test for monosegmental multi-character morphemes
word = Word.from_segments("a b + cd")
assert len(word[1]) == 1
assert word[1][0] == "cd"

# test custom separator & whitespace trimming
word = Word.from_segments("a b = c", separator="=")
assert len(word) == 2
assert str(word) == "a b + c"

word = Word.from_string("a + b + c")

# make sure word can be hashed
Expand Down