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.

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 pure logarithmic grid for candidates search. #461

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

RamSaw
Copy link
Collaborator

@RamSaw RamSaw commented Jun 26, 2023

Now we construct candidates set based on generate_possible_contribution_bounds method that generates numbers that have 3 left-most digits non-zero and others are zero. I.e., it will be [1, 2, 3, ..., 999, 1000, 1010, 1020, ..., 9990, 10000, 10100, 10200.
This is not a pure logarithmic scale, it keeps relative difference between neighbouring almost the same across the candidates set (it varies from 0.1% to 1%). Such solution has a problem that beginning of the sequence can become very scarce due to sampling (we do sampling such sequence contains more than max_candidates values). It is bad because density in the beginning of the sequence should be higher, we need more elements there because a change there in absolute values affects the result much more than change in the right side of the sequence where values are big. For example, difference in result between l_0 = 1 and l_0 = 2 is much bigger than difference between l_0 = 1000 and l_0 = 1001.

Therefore in this PR we implement pure logarithmic grid. The code generates a sequence a_i, where a_i = max_value^(i / (max_candidates - 1)), i in [0..(max_candidates - 1)]

This is copy of #459 but with good diff.

@RamSaw RamSaw requested a review from dvadym June 26, 2023 17:13
Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thanks, it look good! Please fix tests

Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thanks!

@dvadym dvadym merged commit dada1fe into OpenMined:main Jun 29, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants