phoible / dev

PHOIBLE data and development.
https://phoible.org/
GNU General Public License v3.0
115 stars 30 forks source link

update some Ethiopian languages #329

Closed bnk7 closed 3 years ago

bnk7 commented 3 years ago

Addresses, but doesn't fix, issue #299

bambooforest commented 3 years ago

@drammock -- any idea why this doesn't aggregate? been struggling (probably stupidly) for an hour now.

Error in assign_seg_class(dataframe$Phoneme) : 
  all(is_null | is_consonant | is_vowel | is_tone) is not TRUE

on the changes in GM:

https://github.com/phoible/dev/pull/329/commits/57508e5140b39010d33744f5a3c16c3e3c924ee3

triggered here:

https://github.com/phoible/dev/blob/master/scripts/aggregation-helper-functions.R#L134

Wasn't clear to me how to get a nice call on which segment is the culprit -- after eyeballing the segment updates it wasn't clear to me.

drammock commented 3 years ago

There are two NAs sneaking in somehow. They are elements 12284 and 13336 of the variable segment at the point where that function is being called. I figured this out by sourcing the agg script in Rstudio and then clicking "rerun with debug", and looking at:

segment[!(is_null | is_consonant | is_vowel | is_tone)]  # tells me there are NAs
which(is.na(segment))  # tells where they are

that's as deep as I'm able to go at the moment.

bnk7 commented 3 years ago

I had a regular g instead of an IPA g. Hopefully this fixes it!

bambooforest commented 3 years ago

There are extra tabs at those two places in the TSV file @bnk7 .

@drammock -- what's best practice here if I want to add to this PR those tab fixes? I followed the instructions above to check the work:

git checkout -b bnk7-ethiopia_branch master git pull https://github.com/bnk7/dev.git ethiopia_branch

and after I commit and go to push, I get the git suggestion:

git push --set-upstream origin bnk7-ethiopia_branch

but then I end up creating a new PR on my fork. I tried a few other things like pushing to bnk/dev, but I'm doing something wrong. Above in this PR it says:

Add more commits by pushing to the ethiopia_branch branch on bnk7/dev.

But somehow I'm not pushing to that branch.

@bnk7 -- once I figure this out I will add the fixed TSV, add a comment to the Gumer inventory, and add the regenerated aggregated data. Then we can merge.

drammock commented 3 years ago

Here's what I would do:

git remote add bnk7 https://github.com/bnk7/dev.git
git fetch bnk7
git checkout -b ethiopia_branch bnk7/ethiopia_branch
# make whatever changes you need
git add file1 file2 etc
git commit -m 'fix tabs in source'
git push -u bnk7 ethiopia_branch

That will push commits directly to @bnk7's branch, and they'll show up as part of this PR automatically. It only works if @bnk7 adds you as a collaborator on her fork, OR if she ticks this box on the page for this PR:

Screenshot_2021-06-08 fix lingering hard-coded references by drammock · Pull Request #9449 · mne-tools mne-python

drammock commented 3 years ago

The second option is to make a PR from your fork into @bnk7's fork. Same commands as above, except for the last line (origin instead of bnk7):

git push -u origin ethiopia_branch

Then you use the web interface to open a PR with bnk7/ethiopia_branch as the base repository and bambooforest/ethiopia_branch as the head repository, then wait for @bnk7 to merge your PR, at which point those commits become part of her ethiopia_branch and will show up as part of this PR.

bambooforest commented 3 years ago

@drammock -- thanks! I'll have to commit this one to memory.

@bnk7 -- see comments in the PR I sent you. After you merge, this present PR should be ready to merge (including with the latest generated data and updates for #299).