[tonyhu-x]

This PR rewrites the encode_syllables function. The new function (encode_syllables_v2) uses dependency injection instead of hard-coding the syllabification algorithm. The function handles init, read/write operations, and hierarchy update, whereas the actual work to syllabify each word is handled by a SyllabificationAlgo object.

Currently, one SyllabificationAlgo class is implemented, namely MatchOnset. This class is equivalent to the old maxonset algorithm, supporting both derived and custom onsets. Note, however, that the "custom" case is now the default, with derived onsets provided via an alternative initializer from_corpus().

This PR also includes unit tests for MatchOnset and an equivalence test that ensures that the resulting graph from calling encode_syllables_v2 is isomorphic to the one resulting from encode_syllables.

[tonyhu-x]

@mmcauliffe Do we have a pinned minimum Python version that we are planning to support? I don't seem to see it in the code base, but the CI version is pinned to 3.9, which is why tests are failing rn.

[massimolipari]

If we want this to replace encode_syllables() eventually, we'll need to think about how this interfaces with encode_tone_to_syllables() and encode_stress_to_syllables(): both currently contain a hard-coded call to encode_syllables() which forces MaxOnset (see #261).

[mmcauliffe]

@tonyhu-x We can probably bump the testing version to 3.11, since that's the current minimum version for conda forge: https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/main/recipe/conda_build_config.yaml#L995-L999, which looks to be the version adding typing.LiteralString. I'll do a separate PR for it in case it causes any issues with the current codebase and you can pull that in.

To address @massimolipari 's point, it might be better to have add a check for syllables being encoded on those functions and throw an exception if they haven't been? The call to encode_syllables is probably just a convenience things, but I assume most scripts using encode_tone_to_syllables and encode_stress_to_syllables call encode_syllables before them anyway.

[massimolipari]

Hmm, I don't think it's just a convenience thing, I'm pretty sure both those functions actually do require syllables to have already been encoded as is.

[mmcauliffe]

Right they do require it, but I don't think scripts actually call encode_stress_to_syllables just by itself without calls to encode_syllables beforehand. My point is more that if we're going to have multiple syllabification algorithms then any function calls that require syllables should just throw an exception that syllables haven't been encoded and ensure the user is explicitly encoding syllables how they want to, rather than silently just encoding syllables by a default method for them. I think this is also your point as well, I just don't want to unnecessarily bloat the signatures for any syllable-dependent function with things like syllable_algorithm="maxonset" or something.

[massimolipari]

Ah, yes we're saying the same thing then!

I never got as far as figuring out why these functions call encode_syllables() again, but it seemed to have something to do with stripping the tone/stress marker from the phone?

[mmcauliffe]

Left some comments but a couple of them are bigger points across the code.

For naming, we should stick to Max Onset as the name of the algorithm instead of Match Onset, since it's based on the Maximum Onset Principle in the literature. And then using Syllabfier or something a bit more condensed allows for adding it to the end of various algorithm names for class names (i.e. MaxOnsetSyllabifier, SonoritySyllabifier, etc).

For the casting of the query strings to LiteralString, is there a reason do that? I don't think the neo4j query function takes LiteralStrings specifically right? Sticking plain strings might simplify the query calls.

Finally just a couple of questions on implementation details:

1. Did you do any time-based benchmarking of the new version versus old version? Probably need to run it more on larger datasets like the ones in stress testing to get tangible effects
2. Did you play around with newer neo4j functionality to get rid of the speaker/discourse based processing? Same as above, would need a larger dataset for testing effectively

[mmcauliffe]

Merged #271 in for using 3.11 along with controlling it via a repo variable to bump it across all the workflows in the future, if you want to pull that into here, the pytest actions should pass now.

[mmcauliffe]

Ah, yes we're saying the same thing then!

I never got as far as figuring out why these functions call encode_syllables() again, but it seemed to have something to do with stripping the tone/stress marker from the phone?

Ok actually, looking at

PolyglotDB/polyglotdb/corpus/syllabic.py

Line 561 in fb93e6e

self.enrich_syllables(enrich_dict)

, is this what you mean? It calls enrich_syllables, not encode_syllables, where enrich_syllables is adding information taken from the phone labels to the syllables. And then encode_hierarchy is just updating the graph database so that it knows that "tone" or "stress" is available for query on syllables. So I don't think any modification is necessary here, just to close the loop.

[massimolipari]

No, sorry, I should have been more explicit. It's the remove_pattern() call within encode_X_to_syllables() that causes the issue---the very last line is self.encode_syllables("maxonset")

PolyglotDB/polyglotdb/corpus/phonological.py

Lines 114 to 160 in 2b823af

	def remove_pattern(self, pattern="[0-2]"):
	"""
	removes a stress or tone pattern from all phones
	Parameters
	----------
	pattern : str
	the regular expression for the pattern to remove
	Defaults to '[0-2]'
	"""
	phone = getattr(self, self.phone_name)
	if pattern == "":
	pattern = "[0-2]"
	q = self.query_graph(phone)
	results = q.all()
	oldphones = []
	length = 0
	newphones = []
	toAdd = {}
	for item in results:
	phone = item["label"]
	if re.search(pattern, phone) is not None:
	newphone = re.sub(pattern, "", phone)
	length = len(phone) - len(newphone)
	oldphones.append(phone)
	newphones.append(newphone)
	toAdd.update({"label": newphone})
	statement = """MATCH (n:{phone_name}{type}:{corpus_name}) WHERE n.label in $oldphones
	SET n.oldlabel = n.label
	SET n.label=substring(n.label,0,size(n.label)-{length})"""
	norm_statement = statement.format(
	phone_name=self.phone_name,
	type="",
	corpus_name=self.cypher_safe_name,
	length=length,
	)
	type_statement = statement.format(
	phone_name=self.phone_name,
	type="_type",
	corpus_name=self.cypher_safe_name,
	length=length,
	)
	self.execute_cypher(norm_statement, oldphones=oldphones)
	self.execute_cypher(type_statement, oldphones=oldphones)
	self.encode_syllabic_segments(newphones)
	self.encode_syllables("maxonset")

[tonyhu-x]

We also discussed stress testing, and agreed that it is a good idea to add syllabification-specific stress tests. I should investigate and set this up in the near future.