Draft
Conversation
for more information, see https://pre-commit.ci
Contributor
📝 WalkthroughWalkthroughThis PR introduces melting temperature estimation for LAMMPS simulations, refactors molecular dynamics velocity initialization from a boolean disable flag to an optional velocity rescaling factor, adds vmax parameter support for minimization, and introduces XYZ pressure coupling options for NPT ensembles. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MeltingAPI as estimate_melting_temperature
participant Analysis as Structure Analysis
participant MD as MD Calculator<br/>(calc_molecular_dynamics_npt)
participant LAMMPS as LAMMPS Library
User->>MeltingAPI: estimate_melting_temperature(Al, potential, ...)
MeltingAPI->>Analysis: _analyse_minimized_structure()
Analysis->>LAMMPS: Minimize geometry/volume
LAMMPS-->>Analysis: Minimized structure + descriptors
Analysis-->>MeltingAPI: Initial distribution metrics
loop Binary Search Refinement (until temp_step ≤ 10)
MeltingAPI->>MD: _next_calc(structure, T_left)
MD->>LAMMPS: Run NPT MD at T_left
LAMMPS-->>MD: Final structure
MD-->>MeltingAPI: Result structure
MeltingAPI->>Analysis: Compare atom distributions
Analysis-->>MeltingAPI: Distribution mismatch → adjust T_left/T_right
end
MeltingAPI-->>User: Estimated melting temperature (int)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/atomistics/calculators/lammps/melting.py (1)
67-90: Incomplete docstring for_analyse_minimized_structure.The docstring has placeholder Args/Returns sections that are not filled in. Consider completing the documentation or removing the placeholder sections.
📝 Proposed fix to complete the docstring
def _analyse_minimized_structure(structure): """ + Analyze a minimized structure to extract key metrics for melting point estimation. Args: - ham (GenericJob): + structure: Atomistic Structure object after energy minimization. Returns: - + tuple: (structure, key_max, number_of_atoms, distribution_initial_half, final_structure_dict) + - structure: The input structure + - key_max: Key of the dominant structure type + - number_of_atoms: Total number of atoms + - distribution_initial_half: Half of the initial distribution fraction + - final_structure_dict: Full structure analysis dictionary """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/atomistics/calculators/lammps/melting.py` around lines 67 - 90, The docstring for _analyse_minimized_structure is incomplete; update it to describe the function purpose, parameters, and return values: state that it accepts a structure (e.g., an ASE Atoms or pymatgen Structure) and explain the use of _check_diamond and _analyse_structure internally, then document the returned tuple items (structure, key_max: dominant structure key, number_of_atoms: int, distribution_initial_half: float, final_structure_dict: dict) and types, plus any raised exceptions or assumptions; keep wording concise and consistent with other module docstrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ci_support/environment-lammps.yml:
- Line 11: Summary: The dependency specification for pyscal3 has an extra space
after the equals sign causing inconsistent formatting. Fix: update the
dependency line containing "pyscal3 = 3.3.1" to remove the space so it matches
the project's style (i.e., use "pyscal3 =3.3.1"), ensuring consistency with
other lines in the file.
In `@src/atomistics/calculators/lammps/libcalculator.py`:
- Around line 613-620: The type check in _get_vmax_command currently only
accepts float and will wrongly reject integer inputs; change the check to accept
both ints and floats (e.g., use isinstance(vmax, (int, float)) or numbers.Real),
convert vmax to float when formatting into the command, and update the TypeError
message to reflect "vmax must be a number" while keeping the fallback to return
LAMMPS_MINIMIZE_VOLUME unchanged; refer to function _get_vmax_command and
constant LAMMPS_MINIMIZE_VOLUME to locate and modify the logic.
In `@tests/test_melting_lammps.py`:
- Around line 24-33: The test is flaky because estimate_melting_temperature is
called with seed=None, causing non-deterministic results; make the test
deterministic by passing a fixed seed value (e.g., seed=42) into
estimate_melting_temperature and relax the assertion to tolerate observed
variance — replace the strict membership check of melting_temp in [1008, 1023]
with either a range check (e.g., assert melting_temp between lowered_min and
raised_max) or an approximate assertion (e.g., assertAlmostEqual with a
tolerance) referencing the melting_temp variable and the
estimate_melting_temperature call.
---
Nitpick comments:
In `@src/atomistics/calculators/lammps/melting.py`:
- Around line 67-90: The docstring for _analyse_minimized_structure is
incomplete; update it to describe the function purpose, parameters, and return
values: state that it accepts a structure (e.g., an ASE Atoms or pymatgen
Structure) and explain the use of _check_diamond and _analyse_structure
internally, then document the returned tuple items (structure, key_max: dominant
structure key, number_of_atoms: int, distribution_initial_half: float,
final_structure_dict: dict) and types, plus any raised exceptions or
assumptions; keep wording concise and consistent with other module docstrings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 751b75eb-1e97-4b75-92b3-80e9f8fedae6
📒 Files selected for processing (7)
.ci_support/environment-lammps.ymlsrc/atomistics/calculators/lammps/commands.pysrc/atomistics/calculators/lammps/helpers.pysrc/atomistics/calculators/lammps/libcalculator.pysrc/atomistics/calculators/lammps/melting.pytests/test_lammpslib_md.pytests/test_melting_lammps.py
| - iprpy-data =2023.07.25 | ||
| - dynaphopy =1.18.0 | ||
| - lammpsparser =0.0.1 | ||
| - pyscal3 = 3.3.1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of pyscal3 Python package?
💡 Result:
The latest released version of the pyscal3 Python package on PyPI is 3.3.2 (released March 2, 2026). [1]
Source: [1]
🏁 Script executed:
cat -n .ci_support/environment-lammps.ymlRepository: pyiron/atomistics
Length of output: 344
Remove extra space in dependency specification for formatting consistency.
The pyscal3 dependency uses pyscal3 = 3.3.1 with a space after the = sign, while all other dependencies in this file use the format name =version with no space after the equals sign. Standardize this for consistency.
🔧 Proposed fix
-- pyscal3 = 3.3.1
+- pyscal3 =3.3.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.ci_support/environment-lammps.yml at line 11, Summary: The dependency
specification for pyscal3 has an extra space after the equals sign causing
inconsistent formatting. Fix: update the dependency line containing "pyscal3 =
3.3.1" to remove the space so it matches the project's style (i.e., use "pyscal3
=3.3.1"), ensuring consistency with other lines in the file.
Comment on lines
+613
to
+620
| def _get_vmax_command(vmax: Optional[float]) -> str: | ||
| if vmax is not None: | ||
| if isinstance(vmax, float): | ||
| return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax) | ||
| else: | ||
| raise TypeError("vmax must be a float.") | ||
| else: | ||
| return LAMMPS_MINIMIZE_VOLUME |
Contributor
There was a problem hiding this comment.
Type check is too restrictive - rejects valid integer inputs.
The isinstance(vmax, float) check on line 615 will reject integer values like vmax=1, which are valid numeric inputs. In Python, integers are commonly used interchangeably with floats for numeric parameters.
🐛 Proposed fix to accept both int and float
def _get_vmax_command(vmax: Optional[float]) -> str:
if vmax is not None:
- if isinstance(vmax, float):
+ if isinstance(vmax, (int, float)):
return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax)
else:
- raise TypeError("vmax must be a float.")
+ raise TypeError("vmax must be a numeric value (int or float).")
else:
return LAMMPS_MINIMIZE_VOLUME🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/atomistics/calculators/lammps/libcalculator.py` around lines 613 - 620,
The type check in _get_vmax_command currently only accepts float and will
wrongly reject integer inputs; change the check to accept both ints and floats
(e.g., use isinstance(vmax, (int, float)) or numbers.Real), convert vmax to
float when formatting into the command, and update the TypeError message to
reflect "vmax must be a number" while keeping the fallback to return
LAMMPS_MINIMIZE_VOLUME unchanged; refer to function _get_vmax_command and
constant LAMMPS_MINIMIZE_VOLUME to locate and modify the logic.
Comment on lines
+24
to
+33
| melting_temp = estimate_melting_temperature( | ||
| element="Al", | ||
| potential=potential, | ||
| strain_run_time_steps=1000, | ||
| temperature_left=0, | ||
| temperature_right=1000, | ||
| number_of_atoms=8000, | ||
| seed=None, | ||
| ) | ||
| self.assertIn(melting_temp, [1008, 1023]) |
Contributor
There was a problem hiding this comment.
Test is non-deterministic and flaky due to random seed.
The pipeline failure shows melting_temp was 1039, not in the expected [1008, 1023]. With seed=None, a random seed is generated each run, making the test non-deterministic. The expected values list is too narrow for a stochastic simulation.
Consider either:
- Using a fixed seed for reproducibility
- Widening the acceptable range based on observed variance
- Using an approximate assertion (e.g.,
assertAlmostEqualwith a tolerance, or range check)
🐛 Proposed fix using a fixed seed and wider tolerance
melting_temp = estimate_melting_temperature(
element="Al",
potential=potential,
strain_run_time_steps=1000,
temperature_left=0,
temperature_right=1000,
number_of_atoms=8000,
- seed=None,
+ seed=42,
)
- self.assertIn(melting_temp, [1008, 1023])
+ # Melting point estimation has inherent variance; check within reasonable range
+ self.assertGreaterEqual(melting_temp, 900)
+ self.assertLessEqual(melting_temp, 1100)🧰 Tools
🪛 GitHub Actions: Pipeline
[error] 33-33: Test 'test_estimate_melting_temperature' failed: melting_temp 1039 not in expected [1008, 1023].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_melting_lammps.py` around lines 24 - 33, The test is flaky because
estimate_melting_temperature is called with seed=None, causing non-deterministic
results; make the test deterministic by passing a fixed seed value (e.g.,
seed=42) into estimate_melting_temperature and relax the assertion to tolerate
observed variance — replace the strict membership check of melting_temp in
[1008, 1023] with either a range check (e.g., assert melting_temp between
lowered_min and raised_max) or an approximate assertion (e.g., assertAlmostEqual
with a tolerance) referencing the melting_temp variable and the
estimate_melting_temperature call.
prabhath-c
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes the changes in:
Summary by CodeRabbit
Release Notes
New Features
Tests