Return the best (lowest energy) state instead of the final state#31
Return the best (lowest energy) state instead of the final state#31paceheart wants to merge 14 commits into3zcurdia:mainfrom
Conversation
The final state isn't necessarily the best (lowest-energy) state. Now Simulator::run keeps track of the best state as it runs, and returns the best state instead of the happenstance final state.
|
Oops! Sorry for not running the tests first. I'll take a look. |
also factor out a helper method, to get out of ruby jail
…al energy rake just displays the error and moves merrily along, but that's better than nothing
This test (almost always) fails before the "keep track of the best state" patch, and consistently succeeds after.
|
fixed! |
3zcurdia
left a comment
3zcurdia
left a comment
There was a problem hiding this comment.
Thanks for your contribution, but since this gem is being used in exiting projects we need to consider to this change to be available through configuration
Co-authored-by: Luis Ezcurdia <ing.ezcurdia@gmail.com>
Co-authored-by: Luis Ezcurdia <ing.ezcurdia@gmail.com>
Co-authored-by: Luis Ezcurdia <ing.ezcurdia@gmail.com>
"extracting this into a smaller method just to make rubocop happy is unnecessary; we can tell rubocop to skip the cop on this method" -3zcurdia
When true, returns the best (lowest energy) state. When false, returns the final state (preserves old behavior).
I added a new configuration option 'return_best'. I set the default to true, but if you think it's more important to maintain backward compatibility, we can set it to false instead. |
chrisbloom7
left a comment
chrisbloom7
left a comment
There was a problem hiding this comment.
Good suggestion, thanks for the contribution. Left some feedback below.
| 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 |
There was a problem hiding this comment.
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.
| class Configuration | ||
| DEFAULT_COOLING_RATE = 0.0003 | ||
| DEFAULT_INITIAL_TEMPERATURE = 10_000.0 | ||
| DEFAULT_RETURN_BEST = true |
There was a problem hiding this comment.
I think that this should default to false if this will end up going into a minor release. If we want to make it default to true, 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.
3 participants
The final state isn't necessarily the best (lowest-energy) state. Now Simulator::run keeps track of the best state as it runs, and returns the best state instead of the happenstance final state.