-
Notifications
You must be signed in to change notification settings - Fork 302
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.
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
Implement sliding window algorithm for FMD params #4473
Conversation
); | ||
} | ||
|
||
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be amended, because this will always be equal to zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, candidly, i'm working out what index 0 and index 1 correspond to, a short comment would be helpful there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe destructuring the tuple in the method signature? (old_clue_count, new_clue_count): (u64, u64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.1); | |
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! One suggested change inline that we should make prior to merge
); | ||
} | ||
|
||
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.1); | |
let new_clues_in_block = clue_count_delta.1.saturating_sub(clue_count_delta.0); |
I fixed the comments above, and then added a fix in terms of proto naming to correct the fact that the window should be measured in terms of update periods, and not blocks. e.g. a window of "4" means you consider the previous 4 update checks to the parameters, not the previous 4 blocks. (with the default update frequency of 16, this would be 64 blocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! my comments were addressed
Describe your changes
This adds a new algorithm for FMD parameters, which uses a sliding window approximation to target a specified number of detections over a given window. The idea is that if you set a window, of say, 1 day (in blocks), then the algorithm will then approximate the number of clues detected in the past day, and then use that to set the precision, so that the lowest precision such that at least the target number of detections will be generated, if that number of clues is seen over the upcoming window.
We could use a window of 1 day and 1000 detections or so as reasonable production parameters.
Issue ticket number and link
Closes #1087.
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: