more tests on lagrange multipliers#3
Conversation
Summary of ChangesHello @aadityacs, 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 improves the robustness and reliability of the MMA optimization implementation by expanding its test suite. The new tests specifically focus on validating the theoretical properties of Lagrange multipliers and KKT conditions under various constraint and scaling scenarios, ensuring the algorithm behaves as expected in critical optimization contexts. Additionally, a standard .gitignore file has been added for better repository hygiene. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a .gitignore file, removes several debugging print statements, and significantly expands the test suite for Lagrange multipliers in the MMA solver. The new tests are well-designed, covering stationarity conditions for various bound scenarios and scaling properties of the multipliers. My review focuses on suggesting tighter tolerances in the assertions of these new tests to increase their robustness and ensure they can effectively catch subtle inaccuracies in the solver's results.
| self.assertLess( | ||
| residual_norm, | ||
| 0.5, | ||
| msg=f"Stationarity at bounds violated: ||∇f - ξ|| = {residual_norm}", | ||
| ) |
There was a problem hiding this comment.
The tolerance for the stationarity check (0.5) seems quite high. With a kkt_tol of 1e-6 for the solver, a residual norm of up to 0.5 is surprising. The gradient ∇f at the optimal point (1, 1) is [1, 1], so the lower bound multiplier ξ should be very close to this. A large residual norm could mask inaccuracies.
Consider tightening this tolerance to better reflect the expected accuracy of the KKT conditions, for example to 1e-4. If this high tolerance is necessary due to the solver's behavior, a comment explaining why would be helpful.
| self.assertLess( | |
| residual_norm, | |
| 0.5, | |
| msg=f"Stationarity at bounds violated: ||∇f - ξ|| = {residual_norm}", | |
| ) | |
| self.assertLess( | |
| residual_norm, | |
| 1e-4, | |
| msg=f"Stationarity at bounds violated: ||∇f - ξ|| = {residual_norm}", | |
| ) |
| self.assertLess( | ||
| residual_norm, | ||
| 0.5, | ||
| msg=f"Stationarity at upper bounds violated: ||∇f + η|| = {residual_norm}", | ||
| ) |
There was a problem hiding this comment.
Similar to the lower bound test, the tolerance of 0.5 for the stationarity residual norm seems very loose, especially given the kkt_tol of 1e-6. At the optimum (10, 10), ∇f is [-1, -1], so the upper bound multiplier η should be close to [1, 1]. A large residual norm could hide issues.
I recommend tightening this tolerance to ensure the KKT conditions are being met with sufficient precision (e.g., 1e-4). If there's a specific reason for this loose tolerance, it would be good to document it in a comment.
| self.assertLess( | |
| residual_norm, | |
| 0.5, | |
| msg=f"Stationarity at upper bounds violated: ||∇f + η|| = {residual_norm}", | |
| ) | |
| self.assertLess( | |
| residual_norm, | |
| 1e-4, | |
| msg=f"Stationarity at upper bounds violated: ||∇f + η|| = {residual_norm}", | |
| ) |
| self.assertLess( | ||
| relative_error, | ||
| 0.2, # 20% tolerance | ||
| msg=f"Multiplier should scale with objective. " | ||
| f"Expected {expected_lambda:.4f}, got {actual_lambda:.4f}", | ||
| ) |
There was a problem hiding this comment.
A 20% tolerance for the scaling of the Lagrange multiplier seems quite generous and might mask subtle issues. Consider tightening this tolerance to 5% or 10% (e.g., 0.1). If not, adding a comment to explain why such a large tolerance is needed would be beneficial for future maintainers.
| self.assertLess( | |
| relative_error, | |
| 0.2, # 20% tolerance | |
| msg=f"Multiplier should scale with objective. " | |
| f"Expected {expected_lambda:.4f}, got {actual_lambda:.4f}", | |
| ) | |
| self.assertLess( | |
| relative_error, | |
| 0.1, # 10% tolerance | |
| msg=f"Multiplier should scale with objective. " | |
| f"Expected {expected_lambda:.4f}, got {actual_lambda:.4f}", | |
| ) |
| self.assertLess( | ||
| relative_deviation, | ||
| 0.3, # 30% tolerance | ||
| msg="λ * scale should be approximately constant", | ||
| ) |
There was a problem hiding this comment.
The 30% tolerance for checking if λ * scale is constant is very high. This could potentially allow incorrect behavior to pass the test. It would be better to have a much stricter tolerance, like 10% (0.1), to ensure the inverse scaling property holds with reasonable accuracy. If the current solver implementation doesn't allow for a tighter tolerance, it would be valuable to add a comment explaining this limitation.
| self.assertLess( | |
| relative_deviation, | |
| 0.3, # 30% tolerance | |
| msg="λ * scale should be approximately constant", | |
| ) | |
| self.assertLess( | |
| relative_deviation, | |
| 0.1, # 10% tolerance | |
| msg="λ * scale should be approximately constant", | |
| ) |
1 participant
No description provided.