Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement purchase intent classifier for brave ads 8047 #4607

Merged
merged 1 commit into from
Mar 6, 2020
Merged

Implement purchase intent classifier for brave ads 8047 #4607

merged 1 commit into from
Mar 6, 2020

Conversation

moritzhaller
Copy link
Contributor

@moritzhaller moritzhaller commented Feb 12, 2020 •
edited
Loading

Fixes brave/brave-browser#8047

Submitter Checklist:

Test Plan:

  • build up history of intent signals by searching for "audi a6 dealerships nearby" (3 points) and hitting refresh 4 times
    • 10 points are needed for a segment that has a corresponding campaign
    • hitting the same search does increase the intent score each time, this does not work for funnel sites however
    • for testing different searches and sites have a look at keywords.h, try to trigger a match by visiting a listed site or searching for keywords on various search engines. If successful the log should print Purchase intent signal extracted for ...
  • make sure a corresponding campaign is live on staging
    • or alternatively inject a test catalog via charles proxy
  • run nightly with staging flags, e.g. /Applications/Brave\ Browser\ Nightly.app/Contents/MacOS/Brave\ Browser\ Nightly --enable-logging=stderr --log-level=2 --brave-ads-staging --rewards=staging=true
  • wait until browser switches to idle, then move mouse and observe ad notification
    • since contextual + intent ads are drawn from the same pot at random, it can be that a contextual ad is shown first. In that case, repeat process until an intent ad gets triggered

Segment and funnel sites lists + search providers: https://docs.google.com/spreadsheets/d/1RYts5OWZOhT5HUletTKMdRJwTe1p1gjqH43JmplZ5QM/edit#gid=0

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch 5 times, most recently from 7b2d357 to 0ec35d6 Compare February 24, 2020 22:14
@moritzhaller moritzhaller marked this pull request as ready for review February 24, 2020 22:25
@moritzhaller moritzhaller changed the title Draft: Implement purchase intent classifier for brave ads 8047 Implement purchase intent classifier for brave ads 8047 Feb 24, 2020
@moritzhaller moritzhaller self-assigned this Feb 25, 2020
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch 2 times, most recently from b66b8ea to cb2dc92 Compare March 2, 2020 09:25
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch 2 times, most recently from 463b1e4 to c78caf1 Compare March 3, 2020 14:23
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch 2 times, most recently from e7abab4 to bb2ae9b Compare March 3, 2020 16:01
tmancey
tmancey approved these changes Mar 3, 2020
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch from bb2ae9b to 5c1ae1a Compare March 3, 2020 17:23
gdregalo
gdregalo reviewed Mar 3, 2020
View reviewed changes
vendor/bat-native-ads/src/bat/ads/internal/purchase_intent/funnel_sites.h
};

static const std::vector<FunnelSiteInfo> _automotive_funnel_sites = {
FunnelSiteInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need to call constructor explicitly. Use std::initializer_list instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gdregalo like this? FunnelSiteInfo {_funnel_site_segments, "http://autotrader.com", 1}

What's the benefit here? These static lists will be pulled out and refactored with the next piece of work to support "out of band" updates

vendor/bat-native-ads/src/bat/ads/internal/purchase_intent/keywords.cc
bool Keywords::IsSubset(
std::vector<std::string> keyword_set_a,
std::vector<std::string> keyword_set_b) {
std::sort(keyword_set_a.begin(), keyword_set_a.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use std::set instead, if it doesn't break interface agreement? We don't need to sort it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gdregalo I added the existing ticket as a comment. Set impl is planned to come with the next PR as well. Are you happy with that or should we change for this PR?

// TODO(https://github.com/brave/brave-browser/issues/8495): Implement Brave Ads Purchase Intent keyword matching with std::sets

vendor/bat-native-ads/src/bat/ads/internal/purchase_intent/purchase_intent_classifier.cc Outdated
max_segments = history.size();
}

std::vector<std::pair<std::string, uint16_t>> scores;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use std::map or std::multimap instead of vector? It will give us a sorted sequence by default.

@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch from 5c1ae1a to eea45ed Compare March 4, 2020 09:27
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch from eea45ed to a34c332 Compare March 4, 2020 12:41
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch 3 times, most recently from 0993e4a to 13ae2f8 Compare March 5, 2020 13:39
@moritzhaller moritzhaller force-pushed the implement-purchase-intent-classifier-for-brave-ads-8047 branch from 13ae2f8 to 72a2dc0 Compare March 6, 2020 10:47
@moritzhaller moritzhaller merged commit 21fcbe4 into master Mar 6, 2020
@moritzhaller moritzhaller deleted the implement-purchase-intent-classifier-for-brave-ads-8047 branch March 6, 2020 18:52
@bsclifton bsclifton mentioned this pull request Mar 16, 2020
Closed
@btlechowski btlechowski mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdregalo gdregalo gdregalo approved these changes

@tmancey tmancey tmancey approved these changes

Assignees

@moritzhaller moritzhaller

Labels
feature/ads PR/Pending Review
Projects
None yet
Milestone
1.7.x - Release
Development

Successfully merging this pull request may close these issues.

Implement Purchase Intent Classifier for Brave Ads
3 participants
@moritzhaller @tmancey @gdregalo