Integrate emoji parsing; #74

thorbert · 2016-07-31T20:36:04Z

No description provided.

thorbert · 2016-08-03T11:04:26Z

It might be better to separate the AFINN and the emoji lists and let build/index.js build the combined product.

It'd probably also be better to preserve the token order instead of tacking emojis on the end.

…peated emoji

peterpme · 2017-03-15T03:37:52Z

👋 Can I help move this along?

thisandagain · 2017-03-21T13:51:17Z

@peterpme Sure! Here is what I think we need:

~~Add peer reviewed validation dataset which includes emojis~~ (see Add Twitter validation dataset #110)
Check to ensure that changes to the tokenizer included in this PR don't adversely impact performance too much
Resolve merge conflicts
Clean-up a few nitpicks from the PR

thisandagain · 2017-03-21T13:49:38Z

lib/index.js

@@ -34,12 +34,14 @@ module.exports = function (phrase, inject, callback) {
    }

    // Storage objects
-    var tokens      = tokenize(phrase),
+    var tokens      = [],


Why the change here?

thisandagain · 2017-03-21T13:50:03Z

package.json

@@ -19,7 +19,8 @@
    "test": "make test"
  },
  "dependencies": {
-    "lodash.assign": "4.1.0"
+    "lodash.assign": "4.1.0",
+    "spliddit": "^2.1.1"


Please use absolute version numbers (remove the ^)

thisandagain · 2017-03-21T13:52:15Z

build/index.js

@@ -5,35 +5,68 @@
 * @author Andrew Sliwinski <andrew@diy.org>
 */

+/*globals Promise:true*/


I'd rather not introduce promises to this. This should be simple enough to be broken down into smaller functions to avoid callback hell.

thisandagain · 2017-03-21T13:54:00Z

lib/tokenize.js

+var spliddit = require('spliddit');
+
+module.exports = function(input) {
+    if (input === undefined) throw new Error('input undefined');


This module should never throw. I think the expected behavior here would be for the tokenizer to return an empty array.

thisandagain · 2017-03-21T13:59:28Z

lib/tokenize.js

+module.exports = function(input) {
+    if (input === undefined) throw new Error('input undefined');
+    if (input.trim() === '') return [];
+    var tokens = spliddit(input.toLowerCase()


This code path could use some documentation / line comments.

thisandagain · 2017-03-21T14:01:52Z

Resolves #24

thisandagain · 2017-03-21T19:01:38Z

package.json

@@ -19,7 +19,10 @@
  "scripts": {
    "test": "make test"
  },
-  "dependencies": {},
+  "dependencies": {
+    "async": "^2.1.5",


Because async is only used by the build script it should only be included in devDependencies.

thisandagain · 2017-03-21T19:43:43Z

I was able to find a listing of emoji sentiment that we may want to use as the source (similar to how we use AFINN) for the sentiment module. I think I'd be more comfortable landing support with this as the backing dataset.

…NN and Emoji data into the same hash

thisandagain · 2017-03-22T14:08:28Z

build/emoji.txt

@@ -0,0 +1,79 @@
+😁	5


We should drop this file now that we are including Emoji_Sentiment_Data_v1.0.csv

thisandagain · 2017-03-22T14:09:27Z

Thanks @thorbert ! This is really getting close. I think I just have two remaining comments and then we should do final testing.

thisandagain · 2017-03-25T19:17:35Z

Thanks @thorbert ! Just pulling everything down for testing now. Two things I'm observing:

Performance: the new tokenizer has reduced performance by almost 50%
Test coverage: test coverage has dropped from 100% to 87.5% (also due to changes in the tokenizer)

I'll keep digging to see if this might be easy to resolve.

thisandagain · 2017-03-25T23:18:17Z

It appears that the use of spliddit.hasPair is the main performance culprit. Particularly where you are attempting to tokenize emoji that are not delimited by a space (e.g. 😀💩👻). This also happens to be where you don't have test coverage.

I don't really see a way around this completely yet, but having better unicode support for the tokenizer might be worth the performance cost. Here is a quick first shot at attempting to optimize and improve readability using a flat map:

var spliddit = require('spliddit');

/**
 * Applys a "flatten" reducer to the standard JS "map" array method.
 * @param  {Function} lambda Reducer
 * @return {array}           Flattened array
 */
Array.prototype.flatMap = function(lambda) {
    return Array.prototype.concat.apply([], this.map(lambda));
};

/**
 * Remove special characters and return an array of tokens (words).
 * @param  {string} input Input string
 * @return {array}        Array of tokens
 */
module.exports = function(input) {
    // Normalize input
    input = input
        .toLowerCase()
        .replace(/[.,\/#!$%\^&\*;:{}=_`\"~()]/g, '')
        .replace('/ {2,}/',' ');

    // Split on spaces
    input = spliddit(input, ' ');

    // Iterate over tokens and split unicode pairs
    return input.flatMap(function (t) {
        if (!spliddit.hasPair(t)) return t;
        return spliddit(t, '');
    });
};

Another major issue I noticed while testing is that something is amiss with the build script. Common emoji like 😀 and 💩 are included in Emoji_Sentiment_Data_v1.0.csv but are not present in the resulting JSON used by sentiment.

thorbert · 2017-03-26T15:10:16Z

Yes, benchmarks don't look good. I can expand tests but I'm not sure my toy use case makes sense generally (SMS messages from a 4 year old which contain only undelimited emoji). Maybe we can toss that out if doing so helps to restore the performance measures.

thisandagain · 2017-03-29T13:49:20Z

@thorbert Sure. Let's remove that change to the tokenizer. We could always restore it later as a configuration option, but I wouldn't want all users to take on that performance hit.

thisandagain · 2017-03-29T14:03:27Z

lib/tokenize.js

+var spliddit = require('spliddit');
+
+module.exports = function(input) {
+    if (input === undefined) return [];


Because we check if the input if undefined in the containing module (https://github.com/thisandagain/sentiment/blob/develop/lib/index.js#L42), this can be removed.

thisandagain · 2017-03-29T14:03:48Z

lib/tokenize.js

+
+module.exports = function(input) {
+    if (input === undefined) return [];
+    if (input.trim() === '') return [''];


Is your thinking that this is a performance optimization? If so, I think I'd rather track it in a separate PR.

thisandagain · 2017-03-29T14:05:54Z

test/integration/sync_positive_emoji.js

+var test = require('tap').test;
+var sentiment = require('../../lib/index');
+
+var dataset = 'This is so cool ✓';


It would be nice to also include a test case with some more common emoji such as 😄 (that is how I found the build issue).

thisandagain · 2017-04-01T14:01:20Z

This is great! Thanks @thorbert for all your patience as we worked through the details. I'll land this now and then prep a release. 😄 👍

thorbert added 2 commits July 31, 2016 15:48

Integrate emoji parsing

713d379

Add emoji-regex to dependencies

2598f63

thisandagain added the pr - needs review label Aug 2, 2016

thorbert added 2 commits August 3, 2016 07:15

Merge remote-tracking branch 'upstream/develop' into develop

ab614c1

Separate AFINN.txt and emoji.txt; replace tokenizer regex;

cabcdb1

thorbert changed the title ~~Integrage emoji parsing; Add emoji-regex to dependencies;~~ Integrage emoji parsing; Aug 5, 2016

Add some common emoji and guesses at scores to start with

d9075dc

thorbert changed the title ~~Integrage emoji parsing;~~ Integrate emoji parsing; Aug 5, 2016

Add spliddit dependency; Modify tokenizer to split out undelimited/re…

0c2494a

…peated emoji

thisandagain self-assigned this Oct 7, 2016

thisandagain requested changes Mar 21, 2017

View reviewed changes

thisandagain mentioned this pull request Mar 21, 2017

sentiment can now parse emojis #19

Closed

Merge upstream; Address some requested tweaks;

262ffe3

thisandagain reviewed Mar 21, 2017

View reviewed changes

Move async to devDependencies

0a46aea

thisandagain added pr - needs work and removed pr - needs review labels Mar 21, 2017

thisandagain assigned thorbert and unassigned thisandagain Mar 21, 2017

Add Emoji data from Novak, et. al.; Mod build to incorporate both AFI…

89d3f51

…NN and Emoji data into the same hash

thisandagain reviewed Mar 22, 2017

View reviewed changes

thorbert added 3 commits March 23, 2017 15:45

Remove unnecessary emoji.txt

c5beca0

Remove unnecessary output after build

54a2fa0

Put result key count in message to stderr after build.

d6400ba

thisandagain removed the pr - needs work label Mar 25, 2017

thisandagain assigned thisandagain and unassigned thorbert Mar 25, 2017

thisandagain added pr - needs work and removed pr - needs review labels Mar 25, 2017

thisandagain assigned thorbert Mar 25, 2017

thorbert added 3 commits March 26, 2017 10:58

Use codepoint and String.fromCodePoint to parse emoji from backing data

fb5b498

Correct build target in makefile

3e83316

Replace AFINN.json build product after correcting builder

9b7c171

thisandagain mentioned this pull request Mar 29, 2017

Allow tokenizer to split emoji without space delimiter #113

Open

thisandagain requested changes Mar 29, 2017

View reviewed changes

thorbert added 5 commits March 31, 2017 19:00

Tweak eslint settings for corpus to allow double quote and long line

3c571cd

Remove spliddit dependency and reduce complexity in tokenizer

84fca69

Add negative and positive text with emoji tests

47d39fc

Change test name in async_negative.js

9225d82

More paring back of tokenizer

f6a1528

thisandagain unassigned thorbert Apr 1, 2017

thisandagain added pr - needs review and removed pr - needs work labels Apr 1, 2017

thisandagain added pr - good to merge and removed pr - needs review labels Apr 1, 2017

thisandagain approved these changes Apr 1, 2017

View reviewed changes

thisandagain merged commit 5d415b2 into thisandagain:develop Apr 1, 2017

thisandagain mentioned this pull request Apr 1, 2017

Emoji Support #24

Closed

thisandagain mentioned this pull request Mar 31, 2018

Add support for additional languages #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate emoji parsing; #74

Integrate emoji parsing; #74

thorbert commented Jul 31, 2016

thorbert commented Aug 3, 2016

peterpme commented Mar 15, 2017

thisandagain commented Mar 21, 2017 •

edited

Loading

thisandagain Mar 21, 2017

thisandagain Mar 21, 2017

thisandagain Mar 21, 2017

thisandagain Mar 21, 2017 •

edited

Loading

thisandagain Mar 21, 2017

thisandagain commented Mar 21, 2017

thisandagain Mar 21, 2017

thisandagain commented Mar 21, 2017

thisandagain Mar 22, 2017

thisandagain commented Mar 22, 2017 •

edited

Loading

thisandagain commented Mar 25, 2017

thisandagain commented Mar 25, 2017 •

edited

Loading

thorbert commented Mar 26, 2017

thisandagain commented Mar 29, 2017

thisandagain Mar 29, 2017

thisandagain Mar 29, 2017

thisandagain Mar 29, 2017

thisandagain commented Apr 1, 2017

Integrate emoji parsing; #74

Integrate emoji parsing; #74

Conversation

thorbert commented Jul 31, 2016

thorbert commented Aug 3, 2016

peterpme commented Mar 15, 2017

thisandagain commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisandagain Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisandagain commented Mar 21, 2017

Choose a reason for hiding this comment

thisandagain commented Mar 21, 2017

Overview

Dataset

Paper

Choose a reason for hiding this comment

thisandagain commented Mar 22, 2017 • edited Loading

thisandagain commented Mar 25, 2017

thisandagain commented Mar 25, 2017 • edited Loading

thorbert commented Mar 26, 2017

thisandagain commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisandagain commented Apr 1, 2017

thisandagain commented Mar 21, 2017 •

edited

Loading

thisandagain Mar 21, 2017 •

edited

Loading

thisandagain commented Mar 22, 2017 •

edited

Loading

thisandagain commented Mar 25, 2017 •

edited

Loading