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

Add a way to specify a custom move function #2

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

chrisbloom7
Copy link
Collaborator

What

Implements the first proposed change in #1

Why

Metal#swap_collection assumes that the collection is a singe-dimension array and a neighboring state can be found by swapping elements. In my case, I'm working with a two dimensional array and neighboring states are created from shifting elements of one internal array to another. There are likely other use cases that might call for more complicated object manipulations. Being able to specify a custom move function that receives the current state and returns the next state would allow this library to be able to anneal more complex objects.

How

  • Remove assumption that initial state will be an enumerable. It is now referred to as state rather than collection
  • Rename total_energy_calculator to energy_calculator for brevity and consistency
  • Remove default energy_calculator lambda. It assumed a collection of Location objects. Users must now specify their own energy_calculator function based on the object they are annealing.
  • Adds the ability to specify a custom state_change method, replacing Metal#swap_collection. Users must specify their own state_change function based on the object they are annealing.
  • Changes the signature of some methods to use kwargs instead of positional arguments, making it easier to pass in custom energy_calculator or state_change functions
  • Standardizes some variable names
  • Updates to documentation and example bin/run script

Note that the original proposal was:

Add a way to specify a custom move function that defaults to what Metal#swap_collection does now"

I purposely chose not to default to the previous implementation of Metal#swap_collection as that assumed that the initial state was an enumerable, which it isn't guaranteed to be. IMO it's better that we require the user to provide their own method based on whatever object they are annealing.

Copy link
Owner

@3zcurdia 3zcurdia left a comment

Choose a reason for hiding this comment

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

Regarding my changes LGTM

(dx * dx) + (dy * dy)
dx = (x - location.x).abs
dy = (y - location.y).abs
Math.sqrt(dx**2 + dy**2)
Copy link
Owner

Choose a reason for hiding this comment

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

When I implemented this, I also considered to use the same math however calculate an square root is a heavy computation task and for this case it doesn't matter if is a real distance or not because it will compare against other values.

@3zcurdia 3zcurdia merged commit 5007ac7 into 3zcurdia:master Dec 13, 2021
@chrisbloom7 chrisbloom7 deleted the custom-move-function branch January 31, 2022 15:55
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.

2 participants