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

Refactor/kat for sd estimator #177

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Dioprz
Copy link
Collaborator

@Dioprz Dioprz commented Sep 17, 2024

Description

Help required

@Javierverbel @Memphisd @FloydZ

While two of the three functions at tests/external_estimators/SDEstimator/test_sde.py has been succesfully migrated, the function test_all makes a weird correction on some of our internal estimators by using the external one. This is now unsoported as our external and internal estimators don't share the same runtime anymore.

We could probably return the parameters as another input for the internal estimator (like input = (n, k, ...,[<list_of_parameters>])), but I want to be sure this is the best approach we can take.

You can see my renamed version of that test in the last commit (trying to make it a bit easier to read, but I'm not sure if names were apropiately assigned); or look at the original one here:

def test_all():
"""
tests that all estimations match those from https://github.com/Crypto-TII/syndrome_decoding_estimator up to
a tolerance of 0.01 bit
"""
assert len(algos) == len(test_algos)
for i, _ in enumerate(test_algos):
A1 = algos[i]
A2 = test_algos[i]
for set in test_sets:
n, k, w = set[0], set[1], set[2]
Alg = A1(SDProblem(n=n, k=k, w=w), bit_complexities=0)
Alg2 = A2(n=n, k=k, w=w)
# Slight correction of parameter ranges leads to (slightly) better parameters in case of the
# CryptographicEstimators for Both-May and May-Ozerov. For test we fix parameters to the once from the
# online code.
if (
Alg._name == "Both-May"
or Alg._name == "May-OzerovD2"
or Alg._name == "May-OzerovD3"
):
too_much = [
i for i in Alg2["parameters"] if i not in Alg.parameter_names()
]
for i in too_much:
Alg2["parameters"].pop(i)
Alg.set_parameters(Alg2["parameters"])
T1 = Alg.time_complexity()
T2 = Alg2["time"]
assert T2 - ranges <= T1 <= T2 + ranges
.

Review process

  • Not yet

Pre-approval checklist

  • The code builds clean without any errors or warnings
  • I've added/updated tests
  • I've added/updated documentation if needed

@Dioprz Dioprz changed the base branch from main to develop September 17, 2024 06:29
Copy link
Collaborator

@FloydZ FloydZ left a comment

Choose a reason for hiding this comment

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

After some review, we decided, that we drop the enforcement of special parameters for those 3 algorithms Both-May, May-Ozerov-d2, May-Ozerov-D3, And instead we increase the allowed error range for those 3 to 1.5bit.

See here a comparison between the Syndrome Decoding Estimator and the CE for the mentioned parameter sets and algorithms, for justification. But we should keep a comment to explain the problem within the test

# SDE
#BM  100    50 10 16.112675889065212
#BM  1284 1028 24 56.66242774517839
#BM  3488 2720 64 130.95111267584178
#MO2 100    50 10 15.60773200776453
#MO2 1284 1028 24 55.00266278223495
#MO2 3488 2720 64 129.1807953760876
#MO3 100    50 10 17.096776518501812
#MO3 1284 1028 24 56.41499399680342
#MO3 3488 2720 64 129.02721295167652
# CE
#BM  100    50 10 15.777202014956968
#BM  1284 1028 24 55.245646328742176
#BM  3488 2720 64 129.94243715879466
#MO2 100    50 10 15.60773200776453
#MO2 1284 1028 24 55.00266278223495
#MO2 3488 2720 64 129.1807953760876
#MO3 100    50 10 17.096776518501812
#MO3 1284 1028 24 56.231346587683475
#MO3 3488 2720 64 129.02721295167652

@Dioprz
Copy link
Collaborator Author

Dioprz commented Sep 20, 2024

Implemented in 77a30fc. Tests are working as expected now.

What should the comment say, exactly? Comment and better test naming is all that rest for this draft I think

Copy link

sonarcloud bot commented Sep 20, 2024

@FloydZ
Copy link
Collaborator

FloydZ commented Sep 20, 2024

The comment could be:

The CryptographicEstimators finds (slightly) better parameters which lead to slightly better timings in case of Both-May and May-Ozerov. But given the same parameters the CryptographicEstimatos and SyndromeDecodingEstimators compute the same expected runtime.

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