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 local search function #99

Merged
merged 17 commits into from
Dec 11, 2024
Merged

add a local search function #99

merged 17 commits into from
Dec 11, 2024

Conversation

bluest93
Copy link
Contributor

@bluest93 bluest93 commented Nov 6, 2024

Change

Implemented the simple local search algorithm

Description

under core/post_process

  1. Added local_search.py
  • Implements a new class IsingMatrix that represents the Ising model in matrix form (np.array)
    • Converts an IsingModel into matrix form with quadratic and linear components
    • Calculates the energy difference when flipping a bit in the Ising state
  • Implements a new class LocalSearch that perform local search algorithms
    • Runs the local search algorithm until convergence or until a maximum number of iterations
    • Performs first-improvement local search
    • Performs best-improvement local search
    • Decodes the result into a jijmodeling SampleSet
  1. Added __inti__.py
  2. Added tutorial notebook local_search.ipynb

under tests/core/post_process

  1. Added a test file test_local_search.py

@bluest93
Copy link
Contributor Author

For the best_improvement and first_improvement, I choose to return the state instead of index because I think it is easier to understand

@bluest93 bluest93 marked this pull request as ready for review November 15, 2024 09:12
@bluest93
Copy link
Contributor Author

Sorry I found out there is a bug for the result when using best_improvement

Copy link
Collaborator

@Jacomichi Jacomichi left a comment

Choose a reason for hiding this comment

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

Please write the test for all the functionality

qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
@yuyamashiro
Copy link
Contributor

use ruff for format.

Ex.

ruff format qamomile/core/post_process/local_search.py

If you use ruff, please do not format all of the code for the Qamomile project in this Pull Request, as there will be too many changes and the PR will not be able to be reviewed.

qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
qamomile/core/post_process/local_search.py Outdated Show resolved Hide resolved
@bluest93
Copy link
Contributor Author

bluest93 commented Nov 20, 2024

@Jacomichi

Please write the test for all the functionality

I have a confusion for writing a test. For example, the function of run contains other functions _run_local_search, which contains first_improvement and best_improvement, and decode. I guess I need to check all the functions above? but my concern is run consists of _run_local_search and decode(and a error message), testing these two functions seems there is not necessary to do a test on run?

Copy link
Contributor

@yuyamashiro yuyamashiro left a comment

Choose a reason for hiding this comment

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

@bluest93
Please write in the docstring of each method what the function of each method is and how it is intended to be used.
In addition to checking the type, when testing, make sure that it works properly as a local search.

@bluest93
Copy link
Contributor Author

@yuyamashiro For the test_local_search.py, I have added an assertion to check whether the energy after the local search has decreased or is equal to the original energy.

Copy link
Contributor

@yuyamashiro yuyamashiro left a comment

Choose a reason for hiding this comment

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

There is not enough testing and its reliability is still low, but since it is not used in core parts, we will merge it in as is for now.
Also, for local search, the algorithm itself can be improved, such as by efficiently updating it using second-order differences instead of recalculating the energy difference, but we will include these after merging as well.

Copy link
Collaborator

@Jacomichi Jacomichi left a comment

Choose a reason for hiding this comment

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

I left the comments.

tests/core/post_process/test_local_search.py Outdated Show resolved Hide resolved
tests/core/post_process/test_local_search.py Show resolved Hide resolved
tests/core/post_process/test_local_search.py Show resolved Hide resolved
tests/core/post_process/test_local_search.py Outdated Show resolved Hide resolved
docs/tutorial/local_search.ipynb Outdated Show resolved Hide resolved
@Jacomichi Jacomichi self-requested a review December 10, 2024 00:40
@Jacomichi Jacomichi merged commit 4fc3caa into main Dec 11, 2024
6 checks passed
@Jacomichi Jacomichi deleted the Feature/local_search branch December 11, 2024 01:48
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