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

Feat/added the possibility to show the parameters within the table #244

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

FloydZ
Copy link
Collaborator

@FloydZ FloydZ commented Feb 12, 2025

Description

Test script:

from cryptographic_estimators.SDEstimator import SDEstimator
SD = SDEstimator(n=15, k=10, w=5)
SD.table(parameters_inside=True)

@FloydZ FloydZ changed the title added the possibility to show the parameters within the table Feat/added the possibility to show the parameters within the table Feb 12, 2025
Copy link
Collaborator

@Memphisd Memphisd left a comment

Choose a reason for hiding this comment

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

Please use in future branches the branch naming convention, which is lower-case snake_case

I was wondering why this has to be implemented for every estimator separately. I see it is because the table function is overridden in every estimator to enable doctests... maybe for now its ok, but generally we might want to think of something that is easier to maintain, if anyone has an idea.

@FloydZ FloydZ marked this pull request as draft February 17, 2025 08:08
@FloydZ
Copy link
Collaborator Author

FloydZ commented Feb 17, 2025

I was wondering why this has to be implemented for every estimator separately.

I was hoping not. Maybe we could use the args and kwargs arguments. So every table() function within each estimator gets those two arguments. And only the table function in base_estimator gets the actual parameters_inside argument.

@FloydZ FloydZ marked this pull request as ready for review February 21, 2025 14:30
@FloydZ
Copy link
Collaborator Author

FloydZ commented Feb 21, 2025

addressed issues by andre. ready for review

Javierverbel
Javierverbel previously approved these changes Feb 25, 2025
Copy link
Collaborator

@Javierverbel Javierverbel left a comment

Choose a reason for hiding this comment

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

Looks nice. Should we add one or two doctests for this feature?

@Javierverbel Javierverbel force-pushed the Feat/AddParametersIntoTableOutput branch from 8888045 to f32361d Compare March 4, 2025 05:00
@Dioprz Dioprz force-pushed the Feat/AddParametersIntoTableOutput branch from f32361d to 99db65d Compare March 4, 2025 05:46
Copy link

sonarqubecloud bot commented Mar 5, 2025

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.

3 participants