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

Deterministic algorithm benchmark #567

Merged

Conversation

donglinjy
Copy link
Collaborator

Why:
Some algorithm like gridsearch is deterministic when the space is defined, so it make no sense to same task multiple time for the average purpose.

Fix:
For the input algorithms of benchmark,
get_or_create_benchmark(..., algorithms=[...], ...)

Instead of keeping it exactly the same as experiment input
algorithms=["random", {"gridsearch": {"n_values": 50}}]

We change it as below so when some algorithm is deterministic, put True here.

algorithms = [
            {"algorithm": "random"},
            {"algorithm": {"gridsearch": {"n_values": 50}}, "deterministic": True},
        ]

@donglinjy donglinjy requested a review from bouthilx March 3, 2021 14:16
@bouthilx
Copy link
Member

bouthilx commented Mar 8, 2021

The main problem for having deterministic defined in the algorithm class is that Assessment/Study are not working on the instantiated algorithms so they cannot access it, is that it? If we could access at least the algo class based on the algorithm name we could access the class.deterministic property.

@donglinjy
Copy link
Collaborator Author

The main problem for having deterministic defined in the algorithm class is that Assessment/Study are not working on the instantiated algorithms so they cannot access it, is that it? If we could access at least the algo class based on the algorithm name we could access the class.deterministic property.

Correct, current algorithm interfaces is asking for an instance through the factory first, or it is just a string. OptimizationAlgorithm.types and OptimizationAlgorithm.typenames could be a way to access the class object for a string algorithm name.

Although, algorithm level deterministic may not be always accurate, a random seed could make random algorithm reproducible actually.

@bouthilx
Copy link
Member

Although, algorithm level deterministic may not be always accurate, a random seed could make random algorithm reproducible actually.

Good point! I did not think if this at all. So maybe defining if an algorithm is deterministic in the object itself is slightly more complicated.

Perhaps your solution is the simplest one. There is one issue however with supporting str and dict. We should support the dict in the form of {'algo_name': config} as well as {'algorithms': {'algo_name': config}, 'deterministic': True}.

@donglinjy
Copy link
Collaborator Author

@bouthilx we now support all the algorithms input format.

  • ["random"]
  • ["random" :{"seed": 1}]
  • [{"algorithm": "random"}]
  • [{"algorithm": {"random" :{"seed": 1}}}]
  • [{"algorithm": "random", "deterministic": True}]

@bouthilx
Copy link
Member

Alright! There is only few minor things left and then it is ready for merge. Thanks!

Copy link
Member

@bouthilx bouthilx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! :)

@bouthilx bouthilx merged commit 6a9df51 into Epistimio:develop Apr 6, 2021
@bouthilx bouthilx added this to the v0.1.14 milestone Apr 6, 2021
@bouthilx bouthilx added the enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt) label Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves a feature or non-functional aspects (e.g., optimization, prettify, technical debt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants