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

Improve clarity in Sage tests files swapping loads and imports #159

Open
Dioprz opened this issue Jun 16, 2024 · 0 comments
Open

Improve clarity in Sage tests files swapping loads and imports #159

Dioprz opened this issue Jun 16, 2024 · 0 comments
Assignees

Comments

@Dioprz
Copy link
Collaborator

Dioprz commented Jun 16, 2024

Working at test_sdfq.sage my LSP warning me about the next code being inconsistent because we give two inputs to log, but only one is required (as we are really using log2).

from math import comb as binomial, log2 as log

def lee_brickell_correction(k):
    return log(k, 2) * 2 - log(binomial(k, 2), 2)

Which is weird because that is true, but our tests are working.

After some debugging I found the culprit to be the intermediate Sage load functions:

from math import comb as binomial, log2 as log

load("tests/references/helpers/attack_cost.sage")
load("tests/references/helpers/cost.sage")
...

def lee_brickell_correction(k):
    return log(k, 2) * 2 - log(binomial(k, 2), 2)

Because loading is equivalent to execute the Sage file within the global scope, the normal log import used in attack_cost.sage was overriding our log2 as log, allowing all the subsequent code work.

Proposed solution

Since this situation is very tricky, and can easily become a nightmare to debug, we should swap the load and import declarations in every file, making each file completely predictable in its scope.

Previous example would end up like this:

load("tests/references/helpers/attack_cost.sage")
load("tests/references/helpers/cost.sage")

from math import comb as binomial, log2 as log

...

def lee_brickell_correction(k):
    return log(k, 2) * 2 - log(binomial(k, 2), 2)

Which will produce errors, but those are predictable and easy to fix.

I will start making these changes on the files I have access to while making the tests directory refactor, and let the others for later.

@Dioprz Dioprz self-assigned this Jun 17, 2024
Dioprz added a commit that referenced this issue Jul 5, 2024
- Implementing changes proposed in #159.

- Incorrect imports are fixed.

- Remove some testing lines on inner functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant