Add structure optimization for sphinx #415
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request refactors key input-related functions in the SphinxDFT calculator module. The original Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/test_sphinx_parser.py (7)
9-10: Remove unused imports if no longer needed.Although these imports match the refactoring in
sphinxdft.py, please consider removing any constants or functions (e.g.,HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROM) that are not used in this test file, to keep imports clean.
59-60: Rename ambiguous variable.Using
las a loop variable can be confusing. Rename it to something likelinefor better readability.- "".join([l for l in input_sx.split("\n") if "eCut" in l]) + "".join([line for line in input_sx.split("\n") if "eCut" in line])🧰 Tools
🪛 Ruff (0.8.2)
60-60: Ambiguous variable name:
l(E741)
63-64: Rename ambiguous variable.Same reasoning as above. Use a more descriptive variable name instead of
l.- "".join([l for l in input_sx.split("\n") if "folding" in l]) + "".join([line for line in input_sx.split("\n") if "folding" in line])🧰 Tools
🪛 Ruff (0.8.2)
64-64: Ambiguous variable name:
l(E741)
67-68: Rename ambiguous variable.Again, replace
lwith a clearer name for better readability and consistency.- "".join([l for l in input_sx.split("\n") if "potType" in l]) + "".join([line for line in input_sx.split("\n") if "potType" in line])🧰 Tools
🪛 Ruff (0.8.2)
68-68: Ambiguous variable name:
l(E741)
90-105: Consider testing final atomic positions.Currently, the test checks only for the presence of
"structure_with_optimized_positions"in the results dictionary. Including an assertion to verify changes in positions (or a lower final energy) after optimization could strengthen the test.
111-112: Encapsulate repeated parameters.These repeated default arguments for
max_electronic_stepsandenergy_cutoff_in_eVare consistent with code elsewhere, but consider extracting them into constants or fixtures to reduce future duplication.
122-138: Add assertions to verify optimization outcome.Similar to
test_optimize_positions_with_sphinxdft, verifying the final positions or checking that an optimization step actually took place would make this test more robust. Currently, you only check the output dictionary key count.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
atomistics/calculators/sphinxdft.py(8 hunks)tests/test_sphinx_parser.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_sphinx_parser.py
17-17: atomistics.calculators.sphinxdft.HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROM imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
18-18: atomistics.calculators.sphinxdft.BOHR_TO_ANGSTROM imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
60-60: Ambiguous variable name: l
(E741)
64-64: Ambiguous variable name: l
(E741)
68-68: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unittest_qe
- GitHub Check: unittest_matrix (macos-latest, 3.12)
- GitHub Check: unittest_gpaw
- GitHub Check: notebooks
🔇 Additional comments (10)
tests/test_sphinx_parser.py (2)
50-56: Parameter naming consistency check.These parameters, especially
maxStepsandenergy_cutoff_in_eV, align well with the refactored_generate_inputfunction. The refactoring fromworking_directoryto returning the input object is consistent with the new design. Nice job!
106-106: New test function naming looks consistent.Renaming this test method to convey “energy and forces” clarity is a good improvement.
atomistics/calculators/sphinxdft.py (8)
8-8: Great use ofapply_minimization.This import is necessary to integrate the newly introduced optimization routine. Looks good!
24-30: Refactor from_write_inputto_generate_inputis clear.Removing the
working_directoryparameter and returning the constructed Sphinx input object improves flexibility. Good approach!
42-42: Correct unit conversion.Dividing by
HARTREE_TO_EVto convert eV to Hartree is correct. Implementation looks solid.
91-130: New function: position optimization.
- Good job integrating
_generate_inputandapply_minimization.- The final updated positions are correctly multiplied by
1/BOHR_TO_ANGSTROMto switch units to Å.- Consider verifying whether using an initial guess for wavefunctions is beneficial for performance of large systems.
Overall, the functionality and design are consistent with the rest of the refactoring.
137-138: Renamed parameters align with the entire refactor.Switching from
maxStepstomax_electronic_stepsandeCuttoenergy_cutoff_in_eVpreserves clarity across modules.
183-184: Minimal dictionary usage.Initializing
sphinx_optimizer_kwargsto{}ifNoneis a standard Python approach. Good practice.
186-198: Key-based task dispatch is intuitive.Using
"optimize_positions"for conditionally runningoptimize_positions_with_sphinxdftis a straightforward approach. This ensures the new functionality is seamlessly integrated intoevaluate_with_sphinx.
200-212: Graceful handling of tasks.
- The early return for
calc_energy,calc_forces, orcalc_stresskeeps the logic modular.- The fallback
ValueErrorfor unknown tasks provides clarity to users.All looks great here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/test_sphinx_parser.py (7)
8-19: Remove unused importsTwo imported constants are not used in this file:
HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROMBOHR_TO_ANGSTROMConsider removing these imports to keep the code clean.
try: from sphinx_parser.toolkit import to_sphinx from atomistics.calculators.sphinxdft import ( OutputParser, _generate_input, calc_static_with_sphinxdft, optimize_positions_with_sphinxdft, evaluate_with_sphinx, HARTREE_TO_EV, - HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROM, - BOHR_TO_ANGSTROM, )🧰 Tools
🪛 Ruff (0.8.2)
17-17:
atomistics.calculators.sphinxdft.HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROMimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
18-18:
atomistics.calculators.sphinxdft.BOHR_TO_ANGSTROMimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
47-47: Rename test method to match refactored functionSince the
_write_inputfunction has been renamed to_generate_input, this test method should also be renamed for consistency.-def test_write_input(self): +def test_generate_input(self):
59-60: Simplify expression and rename ambiguous variableTwo issues in this line:
- The expression
25.0 * HARTREE_TO_EV / HARTREE_TO_EVsimplifies to25.0- The variable name
lis ambiguous and should be more descriptive- " eCut = " + str(25.0 * HARTREE_TO_EV / HARTREE_TO_EV) + ";", - "".join([l for l in input_sx.split("\n") if "eCut" in l]) + " eCut = 25.0;", + "".join([line for line in input_sx.split("\n") if "eCut" in line])🧰 Tools
🪛 Ruff (0.8.2)
60-60: Ambiguous variable name:
l(E741)
63-64: Rename ambiguous variable in list comprehensionThe variable name
lis ambiguous and should be more descriptive.- "".join([l for l in input_sx.split("\n") if "folding" in l]) + "".join([line for line in input_sx.split("\n") if "folding" in line])🧰 Tools
🪛 Ruff (0.8.2)
64-64: Ambiguous variable name:
l(E741)
67-68: Rename ambiguous variable in list comprehensionThe variable name
lis ambiguous and should be more descriptive.- "".join([l for l in input_sx.split("\n") if "potType" in l]) + "".join([line for line in input_sx.split("\n") if "potType" in line])🧰 Tools
🪛 Ruff (0.8.2)
68-68: Ambiguous variable name:
l(E741)
90-104: Enhance test coverage for structure optimizationThe current test only verifies that the resulting structure has the same number of atoms as the input structure. Consider adding more assertions to verify the correctness of the optimization, such as:
- Checking that the final positions have changed from the initial positions
- Verifying the final energy is lower than the initial energy
- Checking that forces are close to zero after optimization
Also, consider using consistent parameter naming (snake_case) for
dEnergy.def test_optimize_positions_with_sphinxdft(self): structure_result = optimize_positions_with_sphinxdft( structure=self._structure, working_directory=self._output_directory, executable_function=executable_function, max_electronic_steps=100, energy_cutoff_in_eV=25.0 * HARTREE_TO_EV, kpoint_coords=None, kpoint_folding=None, mode="linQN", - dEnergy=1.0e-6, + energy_convergence=1.0e-6, max_ionic_steps=50, ) self.assertEqual(len(structure_result), len(self._structure)) + # Verify positions have changed + self.assertFalse(np.allclose(structure_result.positions, self._structure.positions)) + # Verify forces are close to zero (if available) + # self.assertTrue(np.all(np.isclose(structure_result.get_forces(), 0.0, atol=1e-4)))
121-138: Enhance test coverage and use consistent parameter namingSimilar to the previous test method, the assertions for structure optimization are minimal. Consider adding more comprehensive checks to verify the correctness of the optimization.
Also, there's an inconsistent parameter naming convention in
sphinx_optimizer_kwargs. Consider using snake_case throughout for consistency.sphinx_optimizer_kwargs={ "mode": "linQN", - "dEnergy": 1.0e-6, + "energy_convergence": 1.0e-6, "max_ionic_steps": 50, }, ) self.assertTrue("structure_with_optimized_positions" in results.keys()) self.assertEqual(len(results), 1) +# Verify the optimized structure has the expected number of atoms +self.assertEqual(len(results["structure_with_optimized_positions"]), len(self._structure)) +# Verify positions have changed from the original structure +self.assertFalse(np.allclose( + results["structure_with_optimized_positions"].positions, + self._structure.positions +))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_sphinx_parser.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_sphinx_parser.py
17-17: atomistics.calculators.sphinxdft.HARTREE_OVER_BOHR_TO_EV_OVER_ANGSTROM imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
18-18: atomistics.calculators.sphinxdft.BOHR_TO_ANGSTROM imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
60-60: Ambiguous variable name: l
(E741)
64-64: Ambiguous variable name: l
(E741)
68-68: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10)
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_qe
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_old
- GitHub Check: unittest_matrix (macos-latest, 3.12)
- GitHub Check: unittest_matgl
- GitHub Check: unittest_mace
- GitHub Check: unittest_grace
- GitHub Check: unittest_abinit
- GitHub Check: unittest_gpaw
- GitHub Check: pip_check
- GitHub Check: coverage
- GitHub Check: notebooks
| energy_cutoff_in_eV=25.0 * HARTREE_TO_EV, | ||
| kpoint_coords=None, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify unit conversion in parameter name
The parameter energy_cutoff_in_eV suggests the value is already in eV, but you're multiplying by HARTREE_TO_EV, which appears to be converting from Hartree to eV. Consider either:
- Renaming the parameter to clarify the expected unit
- Removing the conversion factor if the value is already in eV
🏁 Script executed:
#!/bin/bash
# Check the value of HARTREE_TO_EV constant to understand the conversion
rg -A 2 'HARTREE_TO_EV\s*=' -t pyLength of output: 295
Unit Conversion Clarification – Action Required
In the snippet at tests/test_sphinx_parser.py (lines 53–54), the parameter is defined as:
energy_cutoff_in_eV = 25.0 * HARTREE_TO_EV,
kpoint_coords = None,Since HARTREE_TO_EV is set to convert Hartree to eV (as defined in atomistics/calculators/sphinxdft.py), the literal value 25.0 is actually in Hartree. This makes the parameter’s name misleading because it implies the value is already in eV.
Please address one of the following:
- Rename the parameter (e.g., to
energy_cutoff_in_Hartree) to clearly indicate that the input is in Hartree and is subsequently converted to eV. - Remove the conversion factor if the intended behavior is to define the cutoff directly in electronvolts.
Also, remember to update any associated documentation and tests for consistency.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 81.89% 82.04% +0.14%
==========================================
Files 41 41
Lines 2447 2467 +20
==========================================
+ Hits 2004 2024 +20
Misses 443 443 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Refactor