Skip to content

fix: clicks no longer silently deleted when adjacent to a modifier - #387

Open
varunghat wants to merge 2 commits into
phoible:masterfrom
varunghat:fix/click-modifier-typestring
Open

fix: clicks no longer silently deleted when adjacent to a modifier#387
varunghat wants to merge 2 commits into
phoible:masterfrom
varunghat:fix/click-modifier-typestring

Conversation

@varunghat

Copy link
Copy Markdown

Fixes clicks getting silently deleted when reordering an adjacent modifier
(e.g. kǀʰʷ -> kʷʰʷ) in order_ipa()

Clicks were typed the same as modifiers ("M") in make_typestring()
Gave clicks their own typestring code ("K").

Doesn't touch feature-helper-functions.R's separate click="M" typing, so feature computation is unaffected.

codepts[codepts %in% get_codepoints(base_glyphs)] <- "B"
codepts[codepts %in% get_codepoints(modifiers)] <- "M"
codepts[codepts %in% get_codepoints(clicks)] <- "M"
codepts[codepts %in% get_codepoints(clicks)] <- "K"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I checked git blame for this line, which led me to #365. It's PR description says (among other things):

when building feature vectors, clicks are now handled like modifiers (not like base glyphs), which makes it easier to get the correct features (previously they were incorrect)

SO I think this change will likely result in incorrect feature vectors for clicks. I'd rather try changing the line in order_ipa that checks for modifiers %in% mods (see other comment)

for (row in seq_len(dim(ixs)[1])) {
span <- ixs[row,1]:ixs[row,2]
mods <- string[span]
string[span] <- modifiers[modifiers %in% mods]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the correct fix

Suggested change
string[span] <- c(clicks, modifiers)[c(clicks, modifiers) %in% mods]

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