Handle null item times and adjust cost thresholds in `getHeroItemPopularity` #2664

ff137 · 2023-06-23T17:20:05Z

getHeroItemPopularity can error if item.time is null. #2663

Also adjust cost thresholds used in defining early, mid, and late game items. See review comments

ff137 · 2023-06-23T17:26:32Z

Here is the existing logic:

    const startGameItems = countItemPopularity(
      items.filter((item) => item.time <= 0)
    );
    const earlyGameItems = countItemPopularity(
      items.filter(
        (item) => item.time > 0 && item.time < 60 * 10 && item.cost > 700
      )
    );
    const midGameItems = countItemPopularity(
      items.filter(
        (item) =>
          item.time >= 60 * 10 && item.time < 60 * 25 && item.cost > 2000
      )
    );
    const lateGameItems = countItemPopularity(
      items.filter((item) => item.time >= 60 * 25 && item.cost > 4000)
    );

My first proposition is that start game items should be redefined:

    const startGameItems = countItemPopularity(
      items.filter((item) => item.time <= 0 && item.cost <= 600)
    );

because, apparently null <= 0 will return true in js. So, in case a null time is recorded for a lategame item, we can add item.cost <= 600 to filter any of those potential edge cases. 600 is the starting gold.

My other proposition is to adjust the item.cost filters used in mid and late game items. Because, 2000 and 4000 feel like really high thresholds, so will discard a lot of other items, especially for supports. My recommendation would be:

    const midGameItems = countItemPopularity(
      items.filter(
        (item) =>
          item.time >= 60 * 10 && item.time < 60 * 25 && item.cost > 1200
      )
    );
    const lateGameItems = countItemPopularity(
      items.filter((item) => item.time >= 60 * 25 && item.cost > 1800)
    );

@howardchung what do you think?

howardchung · 2023-06-23T17:33:08Z

I feel like if we want to check for null, we should explicitly do that check rather than relying on the 600 gold filter (since that could change)

howardchung · 2023-06-23T17:34:20Z

store/queries.js

@@ -315,7 +315,7 @@ function getHeroItemPopularity(db, redis, heroId, options, cb) {
      .flatMap((purchaseLog) => purchaseLog.purchase_log)

      .map((item) => {
-        const time = parseInt(item.time, 10);
+        const time = item.time ? parseInt(item.time, 10) : 0;


does item?.time work?

Also I think the very next line is going to encounter the same crash when we try item.key. So maybe we should be using filter(Boolean) to remove all null items before we try doing something

item.time ? parseInt(item.time, 10) : 0 will default to 0 when item.time is null

I agree filtering any null would make sense. I'm just curious what would cause the null item.time. If it only ever happens for pre-game items, then defaulting to 0 would make sense. If it can happen to late game items, then the item.cost <= 600 would filter that. But would also make sense to just remove all nulls from the start, assuming it's not too prevalent

if item is null then item.key will crash. I think you want .filter(Boolean) here.

Ok, I added .filter((item) => item !== null && item.key !== null)

null time will then still default to 0, with const time = item.time ? parseInt(item.time, 10) : 0;,
so if it was a pre-game purchase it'll be counted by :

const startGameItems = countItemPopularity( items.filter((item) => item.time <= 0 && item.cost <= 600) );

I still think .filter(Boolean) is simpler and does what we want there. But if you want to check item and item.key specifically then use != to include checking for undefined

In the latest changes I added a filter statement: https://github.com/odota/core/pull/2664/files#diff-6483f8be8db993ad2d2cb82be6da1067d8cb8c925a8559e532e063aefa4f5047R315

We can add filtering to remove cases that have null item.time too. I just can't know what makes most sense, given I can't see how many cases this filters, and what causes those cases.

If item.time defaults to 0, then it can potentially only be counted in starting items, and only if it's less than 600 gold. That feels to me like an adequate solution. But I'm open to just remove null cases for time too, it won't change much

ff137 · 2023-06-23T18:09:41Z

So, I added a filter for null keys, just in case.

I'm trying to inspect cases where time is null, playing around with the /explorer. I finally got GPT to help write a query that runs, but it just times out, even with select 1.

It's basically like this:

SELECT purchase_log
FROM player_matches
WHERE purchase_log::text LIKE '%"time":null%'
LIMIT 1

which times out:
https://api.opendota.com/api/explorer?sql=SELECT%20purchase_log%20FROM%20player_matches%20WHERE%20purchase_log::text%20LIKE%20%27%25%22time%22:null%25%27%20LIMIT%201;

Anyway, sharing that in case you can query without timeout and see an example of a problematic row

ff137 · 2023-06-23T18:24:36Z

store/queries.js

    );
    const earlyGameItems = countItemPopularity(
      items.filter(
-        (item) => item.time > 0 && item.time < 60 * 10 && item.cost > 700
+        (item) => item.time > 0 && item.time < 60 * 10 && item.cost >= 600


>= 600 instead of >700

Essentially will now add: Bottle (675), Voodoo Mask, Ring of Health, Voice Stone (700)

@howardchung last thing I'm wondering about is if there's any justification to reducing this threshold further, maybe to >=500, to include items like boots of speed, bracer, null talisman, wraith band, chainmail.

Reducing to >= 400 would include things like magic wands, buckler, gloves of haste, blades of attack, belt of strength, etc.

So it's not a HUGE amount of items being added by reducing threshold, but it's up to you if it makes sense to include magic wand, blades of attack, etc as "early game items". I don't see why not. I think 500 threshold makes most sense to include bracers/etc. Maybe 450 / 400 if you're okay with including wand, band of elvenskin, robe of magi, etc

Like it may be interesting to see how common boots of speed is in first 10 minutes for a hero. Should be almost every game, but will be differences across heroes. And bracer/wraith/null builds are quite common too, would be interesting for stats to see that too.

ff137 · 2023-06-23T18:28:52Z

store/queries.js

      )
    );
    const midGameItems = countItemPopularity(
      items.filter(
        (item) =>
-          item.time >= 60 * 10 && item.time < 60 * 25 && item.cost > 2000
+          item.time >= 60 * 10 && item.time < 60 * 25 && item.cost >= 1200


>= 1200 instead of >2000

Essentially will now add: Cornucopia, Point Booster (1200), Arcane Boots, Talisman of Evasion (1300), Claymore (1350), Aghs Shard, Platemail, Perseverance, Pavise (1400), Ghost Scepter, Oblivion Staff, Phase Boots (1500), Veil of Discord (1525), Mithril Hammer (1600), Drum of Endurance (1650), Vanguard (1700), Mask of Madness, Mekansm (1775), Dragon Lance (1900), Crystalys (1950), Hyperstone (2000).

ff137 · 2023-06-23T18:30:14Z

store/queries.js

      )
    );
    const lateGameItems = countItemPopularity(
-      items.filter((item) => item.time >= 60 * 25 && item.cost > 4000)
+      items.filter((item) => item.time >= 60 * 25 && item.cost >= 2000)


>= 2000 instead of >4000

Essentially ... will include every item between 2000 and 4000 which was previously ignored

howardchung · 2023-06-25T07:12:18Z

The LIKE query is going to be slow because there isn't a text index on that column.

ff137 · 2023-07-02T16:58:25Z

@howardchung thoughts on these changes?

ff137

@howardchung I noticed that PublicMatchesResponse had some missing params in its definition. So, patching that in this PR

ff137 · 2023-07-02T17:03:54Z

routes/responses/schemas/match/PublicMatchesResponse.js

+      },
+      cluster: {
+        type: "integer",
+      },


howardchung · 2023-07-03T03:56:02Z

My only blocking concern is changing the filter condition to:
.filter((item) => item && item.key)

ff137 · 2023-07-03T10:17:16Z

My only blocking concern is changing the filter condition to: .filter((item) => item && item.key)

How do you mean? I've added: .filter((item) => item !== null && item.key !== null)
Do you want to add item.time !== null?

howardchung · 2023-07-03T16:13:34Z

That depends on the behavior we want if time is null/undefined. If we want to default it to 0, then we shouldn't add it.

But the change I'm asking for is either to just check for item && item.key or item != null && item.key != null to handle undefined (which may not be possible, but I figure it's safer)

ff137 · 2023-07-03T16:25:42Z

store/queries.js

      .flatMap((purchaseLog) => purchaseLog.purchase_log)
-
+      .filter((item) => item && item.key && item.time)


@howardchung I edited as follows

makes sense to not need !== null check - I'm new to javascript so wasn't sure on diff between the two, but asserting truthiness is probably better than only asserting not null. Filtering by time also makes sense because should be an anomaly

howardchung · 2023-06-25T17:00:04Z

store/queries.js

      .flatMap((purchaseLog) => purchaseLog.purchase_log)
-
+      .filter((item) => item !== null && item.key !== null)


I think you should use != here to catch undefined in addition to null.

ff137 · 2023-07-03T16:29:27Z

@howardchung thanks 👍

🎨 handle null item purchase times

1ce73db

ff137 mentioned this pull request Jun 23, 2023

itemPopularity endpoint fails for some heroes #2663

Closed

howardchung reviewed Jun 23, 2023

View reviewed changes

ff137 added 3 commits June 23, 2023 19:47

filter null items keys

c9c3121

starting items cost must be <= starting gold

8ddec76

🎨

80d0b32

ff137 added 3 commits June 23, 2023 20:21

set early-game item cost to >= 600

42784a4

set mid-game item cost to >= 1200

4cea609

set late-game item cost to >= 2000

08923b0

ff137 commented Jun 23, 2023

View reviewed changes

ff137 changed the title ~~Handle null item purchase times~~ Handle null item times and adjust cost thresholds in getHeroItemPopularity Jun 23, 2023

ff137 added 7 commits June 25, 2023 10:42

filter so that item !== null

820e645

Merge branch 'master' into fix/hero-item-popularity-query

9b8693d

patch version bump

4ba98dc

update package-lock.json

a05530a

update spec

adc5cc9

reduce earlyGame item cost threshold to >= 500

e6d4de4

reduce midGame item cost threshold to >= 1000

fac1f13

ff137 requested a review from howardchung June 25, 2023 08:59

ff137 added 2 commits July 2, 2023 19:01

add missing response params to PublicMatchesResponse

b99ea09

updated spec

4e8244d

ff137 commented Jul 2, 2023

View reviewed changes

routes/responses/schemas/match/PublicMatchesResponse.js

},

cluster: {

type: "integer",

},

Copy link

Contributor Author

ff137 Jul 2, 2023

Choose a reason for hiding this comment

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

here*

🎨 add time check to filter to assert not null

1231b83

ff137 commented Jul 3, 2023

View reviewed changes

howardchung approved these changes Jul 3, 2023

View reviewed changes

howardchung merged commit 50320c3 into odota:master Jul 3, 2023

ff137 deleted the fix/hero-item-popularity-query branch August 13, 2023 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle null item times and adjust cost thresholds in `getHeroItemPopularity` #2664

Handle null item times and adjust cost thresholds in `getHeroItemPopularity` #2664

ff137 commented Jun 23, 2023 •

edited

Loading

ff137 commented Jun 23, 2023 •

edited

Loading

howardchung commented Jun 23, 2023

howardchung Jun 23, 2023

ff137 Jun 23, 2023

ff137 Jun 23, 2023

howardchung Jun 25, 2023

ff137 Jun 25, 2023

ff137 Jun 25, 2023 •

edited

Loading

howardchung Jun 25, 2023

ff137 Jun 25, 2023

ff137 Jun 25, 2023

ff137 commented Jun 23, 2023 •

edited

Loading

ff137 Jun 23, 2023 •

edited

Loading

ff137 Jun 25, 2023 •

edited

Loading

ff137 Jun 25, 2023

ff137 Jun 23, 2023 •

edited

Loading

ff137 Jun 23, 2023 •

edited

Loading

howardchung commented Jun 25, 2023

ff137 commented Jul 2, 2023

ff137 left a comment

ff137 Jul 2, 2023

howardchung commented Jul 3, 2023

ff137 commented Jul 3, 2023

howardchung commented Jul 3, 2023

ff137 Jul 3, 2023

ff137 Jul 3, 2023

howardchung Jun 25, 2023

ff137 commented Jul 3, 2023

		.flatMap((purchaseLog) => purchaseLog.purchase_log)

		.filter((item) => item && item.key && item.time)

		.flatMap((purchaseLog) => purchaseLog.purchase_log)

		.filter((item) => item !== null && item.key !== null)

Handle null item times and adjust cost thresholds in getHeroItemPopularity #2664

Handle null item times and adjust cost thresholds in getHeroItemPopularity #2664

Conversation

ff137 commented Jun 23, 2023 • edited Loading

ff137 commented Jun 23, 2023 • edited Loading

howardchung commented Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ff137 Jun 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ff137 commented Jun 23, 2023 • edited Loading

ff137 Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

ff137 Jun 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ff137 Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

ff137 Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

howardchung commented Jun 25, 2023

ff137 commented Jul 2, 2023

ff137 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardchung commented Jul 3, 2023

ff137 commented Jul 3, 2023

howardchung commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ff137 commented Jul 3, 2023

Handle null item times and adjust cost thresholds in `getHeroItemPopularity` #2664

Handle null item times and adjust cost thresholds in `getHeroItemPopularity` #2664

ff137 commented Jun 23, 2023 •

edited

Loading

ff137 commented Jun 23, 2023 •

edited

Loading

ff137 Jun 25, 2023 •

edited

Loading

ff137 commented Jun 23, 2023 •

edited

Loading

ff137 Jun 23, 2023 •

edited

Loading

ff137 Jun 25, 2023 •

edited

Loading

ff137 Jun 23, 2023 •

edited

Loading

ff137 Jun 23, 2023 •

edited

Loading