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

Fixes AC for 1min setting #1507

Merged
merged 4 commits into from
Feb 13, 2019
Merged

Fixes AC for 1min setting #1507

merged 4 commits into from
Feb 13, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jan 30, 2019 •
edited
Loading

Resolves brave/brave-browser#3320

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  • covered with automated tests

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

@NejcZdovc NejcZdovc added this to the 0.62.x - Nightly milestone Jan 30, 2019
@NejcZdovc NejcZdovc self-assigned this Jan 30, 2019
@NejcZdovc NejcZdovc force-pushed the score-fix-test branch 3 times, most recently from 1d0f707 to f14a068 Compare February 6, 2019 19:19
NejcZdovc
NejcZdovc commented Feb 12, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat_publishers.cc Outdated
uint64_t min_duration_ms = min_duration *
braveledger_ledger::_milliseconds_second;
a_ = (1.0 / (braveledger_ledger::_d * 2.0)) - min_duration_ms;
uint64_t min_duration_big = min_duration * 100;
Copy link
Contributor Author

@NejcZdovc NejcZdovc Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we needed to reduce this from 1000 to 100 as we were hitting limit and getting nan. We still multiply it with 100 to keep precision

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused, are you talking about the JS side? Presumably multiplying by 1000 was for ms, so why would we multiply by 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct 1000 was for ms. If we remove multiply completely formula is not that efficient anymore, so that's why we still multiply it with 100 to have better results

NejcZdovc
NejcZdovc commented Feb 12, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat_publishers.cc Outdated
b2_ = b_ * b_;
}

// courtesy of @dimitry-xyz: https://github.com/brave/ledger/issues/2#issuecomment-221752002
double BatPublishers::concaveScore(const uint64_t& duration) {
uint64_t duration_ms = duration * braveledger_ledger::_milliseconds_second;
return (-b_ + std::sqrt(b2_ + (a4_ * duration_ms))) / a2_;
uint64_t duration_big = duration * 100;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@NejcZdovc NejcZdovc force-pushed the score-fix-test branch 6 times, most recently from 29a4bea to 0890027 Compare February 12, 2019 09:14
@NejcZdovc NejcZdovc changed the title Adds test for score calculation Fixes AC for 1min setting Feb 12, 2019
NejcZdovc
NejcZdovc commented Feb 12, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat_publishers.cc
@@ -431,6 +430,7 @@ void BatPublishers::OnRestorePublishersInternal(bool success) {

void BatPublishers::setPublisherMinVisitTime(const uint64_t& duration) { // In seconds
state_->min_publisher_duration_ = duration;
calcScoreConsts(duration);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to update variables on the fly and not only when we restart the browser

Copy link
Collaborator

@bridiver bridiver Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we should really move these things to an observer for PublisherMinVisitTime change in a follow-up somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so ledger would save data and call rewards that save was successful and we would trigger observer that would then call back ledger?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, an observer internal to ledger

bridiver
bridiver reviewed Feb 12, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat_helper.cc
@@ -607,7 +607,7 @@ static bool ignore_ = false;
allow_videos_ = state.allow_videos_;
monthly_balances_ = state.monthly_balances_;
recurring_donation_ = state.recurring_donation_;
migrate_score = state.migrate_score;
migrate_score_2 = state.migrate_score_2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

score_2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we migrated once already and we just need to migrate it again, so that's why I rename it to 2 as 1 is not needed anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

but why does it need to be score_2 in the data structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that we migrate only once

Copy link
Collaborator

@bridiver bridiver Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see, the name has changed now, but I think what we need here is a version attribute for the data

@NejcZdovc NejcZdovc requested a review from bridiver February 13, 2019 11:57
ryanml
ryanml previously approved these changes Feb 13, 2019
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

lgtm

@NejcZdovc NejcZdovc merged commit 702740c into master Feb 13, 2019
@NejcZdovc NejcZdovc deleted the score-fix-test branch February 13, 2019 20:54
NejcZdovc added a commit that referenced this pull request Feb 13, 2019
@NejcZdovc
Merge pull request #1507 from brave/score-fix-test
c5754b0 
Fixes AC for 1min setting
@NejcZdovc NejcZdovc mentioned this pull request Feb 13, 2019
Merged
NejcZdovc added a commit that referenced this pull request Feb 13, 2019
@NejcZdovc
Merge pull request #1507 from brave/score-fix-test
bb38af4 
Fixes AC for 1min setting
@NejcZdovc NejcZdovc mentioned this pull request Feb 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@ryanml ryanml ryanml left review comments

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

@mrose17 mrose17 Awaiting requested review from mrose17

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.62.x - Release
Development

Successfully merging this pull request may close these issues.

AC table not working if min visit is set to 1min
3 participants
@NejcZdovc @bridiver @ryanml