Conversation
Summary of ChangesHello @wd7512, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of the simulated annealing framework by decoupling the optimization algorithm from specific objective functions. It achieves this by introducing a mechanism to pass any 2D function to the SA solver, alongside a modular restructuring of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@copilot finish this MR |
|
@Will-D-AER I've opened a new pull request, #18, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Code Review
This pull request aims to make the objective function for the simulated annealing algorithm adjustable, which is a valuable enhancement. The code has been modularized in both Python and Rust by moving the rastrigin_2d function into separate math modules, and an unused serial Rust algorithm has been removed. However, there are several critical issues that need to be addressed. The Python implementation is currently broken as the new function parameter is not correctly passed through the call stack, which will lead to runtime errors. The Rust implementation has not been updated to accept a generic function, contrary to the PR's objective. Finally, a new test file has been added but it is incomplete and contains syntax errors. I've left specific comments on the areas that need fixing.
Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
Add pluggable objective functions with quadratic benchmark
|
@copilot adjust so that we can define which function we want to use in the run_experiment.py file. this is as simple as adding loss_function as an input to the train_tuner. Also ensure the wording of loss_function is accurately used across the whole codebase. Pick a better more appropriate name than "loss_function" |
|
@Will-D-AER I've opened a new pull request, #19, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
…sistent defaults Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
…rameter Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
Co-authored-by: Will-D-AER <197624355+Will-D-AER@users.noreply.github.com>
Add configurable objective function parameter with full Rust support
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of making the objective function adjustable, which significantly increases the flexibility of the optimization framework. The modularization of both Python and Rust code is clean, and the addition of tests to ensure implementation equivalence is an excellent practice. My review includes a few suggestions to further improve code robustness, style, and performance.
| if function is not None: | ||
| fn_name = getattr(function, "__name__", None) | ||
| if fn_name == "rastrigin_2d": | ||
| function_name = "rastrigin" | ||
| elif fn_name == "quadratic_2d": | ||
| function_name = "quadratic" | ||
| else: | ||
| raise ValueError( | ||
| f"Rust parallel implementation only supports rastrigin_2d and quadratic_2d, " | ||
| f"but received '{fn_name}'." | ||
| ) |
There was a problem hiding this comment.
Relying on __name__ to identify functions can be brittle. For example, if the function is a lambda or has been wrapped by a decorator, its __name__ might not be what you expect. A more robust approach is to check for function identity. This would require importing the functions from core.math. The local import in the suggestion can be moved to the top of the file.
| if function is not None: | |
| fn_name = getattr(function, "__name__", None) | |
| if fn_name == "rastrigin_2d": | |
| function_name = "rastrigin" | |
| elif fn_name == "quadratic_2d": | |
| function_name = "quadratic" | |
| else: | |
| raise ValueError( | |
| f"Rust parallel implementation only supports rastrigin_2d and quadratic_2d, " | |
| f"but received '{fn_name}'." | |
| ) | |
| if function is not None: | |
| from core.math import rastrigin_2d, quadratic_2d | |
| if function is rastrigin_2d: | |
| function_name = "rastrigin" | |
| elif function is quadratic_2d: | |
| function_name = "quadratic" | |
| else: | |
| fn_name = getattr(function, "__name__", "<unknown>") | |
| raise ValueError( | |
| f"Rust parallel implementation only supports rastrigin_2d and quadratic_2d, " | |
| f"but received '{fn_name}'." | |
| ) |
| Returns: | ||
| The objective function | ||
| """ | ||
| from core import math as math_funcs |
There was a problem hiding this comment.
According to PEP 8, imports should generally be at the top of the file. This local import can be moved to the top level as there doesn't appear to be a risk of circular dependencies. This improves readability and maintainability.
Please move from core import math as math_funcs to the top of the file and remove this line.
| try: | ||
| import sa_rust | ||
|
|
||
| rust_available = True | ||
| except ImportError: | ||
| print( | ||
| "Warning: sa_rust module not available. Run 'uv run maturin develop --release' first." | ||
| ) | ||
| rust_available = False |
There was a problem hiding this comment.
For handling optional dependencies in tests, it's more idiomatic to use pytest.importorskip. This will correctly mark tests as 'skipped' in test reports if the module isn't found, which is more informative than printing a warning and having the test pass vacuously.
To implement this, you can:
- Add
import pytestto the top of the file. - Replace this
try...exceptblock withsa_rust = pytest.importorskip("sa_rust"). - Remove the
if not rust_available:checks from bothtest_rastrigin_equivalenceandtest_quadratic_equivalence.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Allows for any function to be used in place of the rastrigin function. As long as it maps x, y -> z.
Tests added to ensure rust implementation and python implementation is the same.
Add 1, more simple function, a simple quadratic z = x**2 + y ** 2.
Modularises rust code
Modularises python code
removes unused sa rust algorithm