-
Notifications
You must be signed in to change notification settings - Fork 2
Return the best (lowest energy) state instead of the final state #31
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
Open
paceheart
wants to merge
14
commits into
3zcurdia:main
Choose a base branch
from
paceheart:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
593d65c
refactor to expose lower_energy? method
paceheart 4f2d654
keep track of the best (lowest energy) state and return it
paceheart ec68051
revert accidental comment edit
paceheart 6458980
revert accidental comment edit
paceheart 4efdc44
remove whitespace
paceheart a9c55fc
preserve the final temperature in the return value
paceheart 96275de
raise a RuntimeError if the solution energy is greater than the initi…
paceheart b44dec4
add test_finds_optimal_solution
paceheart 4e83021
remove unnecessary comment; the function is self explanatory
paceheart 622f887
use || instead of 'or'
paceheart 124f149
improve readability
paceheart ea26f98
merge really_run back into run
paceheart eaa2100
Merge branch 'main' of https://github.com/paceheart/annealing
paceheart 99a63d5
Add return_best configuration option
paceheart File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,5 +123,44 @@ def test_passes_run_time_configuration_to_metal | |
| energy_calculator.verify | ||
| state_changer.verify | ||
| end | ||
|
|
||
| def real_energy_calculator(ary) | ||
| ary[-1] | ||
| end | ||
|
|
||
| def test_finds_optimal_solution_when_return_best | ||
| initial_energy = real_energy_calculator(@collection) | ||
| final_metal = @simulator.run(@collection, | ||
| energy_calculator: ->(x) { real_energy_calculator(x) }, | ||
| state_change: ->(state) { state.shuffle }, | ||
| return_best: true) | ||
| final_energy = real_energy_calculator(final_metal.state) | ||
|
|
||
| assert final_energy < initial_energy | ||
| # we gave it plenty of time to find the optimal solution | ||
| assert_equal 1, final_energy | ||
| end | ||
|
|
||
| def test_finds_suboptimal_solution_when_not_return_best | ||
| max_iterations = 10 | ||
| initial_energy = real_energy_calculator(@collection) | ||
| final_energy = 1 | ||
| iteration_count = 0 | ||
|
|
||
| # The simulator might happen to finish on the optimal state by random chance. | ||
| # If that happens, run it again up to MAX_ITERATIONS times. | ||
| until final_energy > 1 || iteration_count > max_iterations | ||
| iteration_count += 1 | ||
| final_metal = @simulator.run(@collection, | ||
| energy_calculator: ->(x) { real_energy_calculator(x) }, | ||
| state_change: ->(state) { state.shuffle }, | ||
| return_best: false) | ||
| final_energy = real_energy_calculator(final_metal.state) | ||
|
|
||
| assert final_energy < initial_energy | ||
| end | ||
|
|
||
| refute_equal 1, final_energy | ||
| end | ||
|
Comment on lines
+144
to
+164
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's still a chance this test could fail after max_attempts since it's just relying on random chance. I'd prefer a test that is deterministic, even if that means using stubs or mocks to setup the specific scenario this test is exercising. |
||
| end | ||
| end | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should default to
falseif this will end up going into a minor release. If we want to make it default totrue, then we should def give a major version bump and I'd expect to see documentation about the upgrade path to keep the old functionality. I defer to @3zcurdia about which route he would prefer to see.