Workflow to estimate melting point with LAMMPS #610
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughParameterizes initial LAMMPS velocity scaling via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Estimate as "estimate_melting_temperature"
participant Minimizer as "Minimizer"
participant Analyzer as "StructureAnalyzer"
participant MD as "MD_NPT"
participant Bisection as "_next_step_funct"
User->>Estimate: request(structure, potential, T_left, T_right, target_atoms)
Estimate->>Minimizer: build & minimize initial structure
Minimizer-->>Estimate: minimized_structure
Estimate->>Analyzer: analyze minimized_structure
Analyzer-->>Estimate: initial_distribution
loop until (T_right - T_left) ≤ tolerance
Estimate->>Bisection: current bounds, structures, distribution
Bisection->>MD: run NPT MD at selected temperature
MD-->>Bisection: md_result
Bisection->>Minimizer: minimize md_result
Minimizer-->>Bisection: minimized_md
Bisection->>Analyzer: analyze minimized_md
Analyzer-->>Bisection: new_distribution
Bisection-->>Estimate: updated bounds & structures
end
Estimate-->>User: final estimated melting temperature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/atomistics/calculators/lammps/melting.py (4)
67-75: Incomplete docstring with stale content.The docstring references
ham (GenericJob)which doesn't match the actual parameterstructure. The Args and Returns sections are empty.📝 Suggested docstring update
def _analyse_minimized_structure(structure): """ + Analyze a minimized structure to determine its dominant crystal type. Args: - ham (GenericJob): + structure (ase.Atoms): Atomistic structure object to analyze. 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 fraction of atoms in the dominant type + - final_structure_dict: Full descriptor dictionary """
93-108: Docstring contains irrelevant parameter descriptions.The docstring mentions parameters like
job_type,project,potential, andqueuethat don't exist in this function's signature. This appears to be carried over from a different codebase (pyiron_atomistics).📝 Suggested docstring cleanup
def _next_calc(structure, potential, temperature, seed, run_time_steps=10000): """ - Calculate NPT ensemble at a given temperature using the job defined in the project parameters: - - job_type: Type of Simulation code to be used - - project: Project object used to create the job - - potential: Interatomic Potential - - queue (optional): HPC Job queue to be used + Run NPT molecular dynamics at a given temperature using LAMMPS. Args: - structure (pyiron_atomistics.structure.atoms.Atoms): Atomistic Structure object to be set to the job as input sturcture + structure (ase.Atoms): Atomistic structure object for the simulation. + potential: Interatomic potential dataframe. temperature (float): Temperature of the Molecular dynamics calculation + seed (int): Random seed for velocity initialization. run_time_steps (int): Number of Molecular dynamics steps Returns: - Final Atomistic Structure object + ase.Atoms: Final structure after MD equilibration. """
133-219: Bisection logic for melting point estimation looks correct.The control flow handles the three expected scenarios: both crystalline (shift right), left crystalline/right melted (bisect), both melted (shift left). The
ValueErroron line 218 is appropriate as a defensive assertion for an unreachable state.The docstring is incomplete (empty Args/Returns), similar to
_analyse_minimized_structure.
294-294: Redundantint()call.In Python 3,
round()with a single argument already returns an integer, making the outerint()call unnecessary.♻️ Suggested fix
- return int(round(temperature_left)) + return round(temperature_left)src/atomistics/calculators/lammps/libcalculator.py (1)
613-620: Type check is overly restrictive.The
isinstance(vmax, float)check will reject integer values like1or expressions that evaluate to integers. Since LAMMPS accepts numeric values forvmax, consider accepting bothintandfloat.♻️ Suggested fix
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.") else: return LAMMPS_MINIMIZE_VOLUME
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/atomistics/calculators/lammps/commands.pysrc/atomistics/calculators/lammps/helpers.pysrc/atomistics/calculators/lammps/libcalculator.pysrc/atomistics/calculators/lammps/melting.pytests/test_lammpslib_md.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/atomistics/calculators/lammps/libcalculator.py (1)
src/atomistics/shared/output.py (2)
OutputMolecularDynamics(42-51)keys(11-18)
🪛 Ruff (0.14.10)
src/atomistics/calculators/lammps/melting.py
218-218: Avoid specifying long messages outside the exception class
(TRY003)
232-232: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
294-294: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
src/atomistics/calculators/lammps/libcalculator.py
160-160: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
240-240: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
327-327: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
403-403: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
618-618: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_qe
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_gpaw
- GitHub Check: notebooks
- GitHub Check: coverage
🔇 Additional comments (12)
src/atomistics/calculators/lammps/helpers.py (1)
105-119: LGTM!The
velocity_rescale_factorparameter is cleanly integrated into the thermal expansion loop. It's properly added to the function signature with a sensible default value of 2.0 and correctly propagated to the template rendering context.src/atomistics/calculators/lammps/melting.py (2)
17-36: LGTM!The diamond detection heuristic is reasonable—comparing the "OTHER" counts from CNA vs diamond descriptors to determine which analysis method better classifies the structure.
39-64: LGTM!Clean dispatcher function with appropriate documentation.
tests/test_lammpslib_md.py (2)
74-74: LGTM!The test correctly uses
velocity_rescale_factor=Noneto disable initial velocity assignment, aligning with the updated API. The assertion that temperature stays below 1 correctly verifies the expected behavior.
166-166: LGTM!Consistent with the NVT test update—correctly uses
velocity_rescale_factor=Nonefor the NPT no-velocity test case.src/atomistics/calculators/lammps/commands.py (1)
23-23: LGTM!The velocity command template correctly replaces the hardcoded factor
2with the parameterized{{ velocity_rescale_factor }}. The LAMMPSvelocity all createcommand syntax with the arithmetic expression$(...)is valid.src/atomistics/calculators/lammps/libcalculator.py (6)
52-57: LGTM!The
vmaxparameter is cleanly integrated with proper Optional typing and delegates command generation to the_get_vmax_commandhelper.
158-184: LGTM!The
velocity_rescale_factorparameter is properly integrated. WhenNone, the velocity command is correctly omitted from the template. The branching logic is clean and the parameter is properly passed to the template render.
237-257: LGTM!The
couple_xyzoption is cleanly implemented. Consider using lowercase for the local variable (lammps_ensemble_npt_xyz) to distinguish it from module-level constants, but this is a minor style preference.
325-351: LGTM!Consistent implementation of
velocity_rescale_factorparameter following the same pattern as NVT and NPT functions.
401-429: LGTM!Consistent
velocity_rescale_factorimplementation for Langevin dynamics.
486-505: LGTM!The
couple_xyzparameter is correctly integrated. The function uses the defaultvelocity_rescale_factor=2.0fromlammps_thermal_expansion_loop, which is appropriate for thermal expansion calculations.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
==========================================
+ Coverage 86.81% 86.92% +0.10%
==========================================
Files 43 44 +1
Lines 2419 2500 +81
==========================================
+ Hits 2100 2173 +73
- Misses 319 327 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/test_melting_lammps.py:
- Around line 24-32: The test is non-deterministic because
estimate_melting_temperature is called with seed=None which yields random
results; update the test call in tests/test_melting_lammps.py to pass a fixed
integer seed (e.g., seed=42) to estimate_melting_temperature and then re-run the
test to capture the deterministic expected melting_temp value and update the
assertion to that single expected value; reference the
estimate_melting_temperature call and the seed handling in melting.py (around
where seed is checked/initialized) when making the change.
🧹 Nitpick comments (2)
tests/test_melting_lammps.py (2)
33-33: Document the expected values in the assertion.The assertion accepts two specific values without explanation. Add a comment clarifying why these values are expected (e.g., known melting temperature range for Al with this potential, tolerance for numerical precision, etc.).
📝 Proposed documentation
- self.assertIn(melting_temp, [1008, 1023]) + # Expected melting temperature for Al with Mishin-1999 potential: ~1008-1023 K + # (experimental Al melting point: 933 K; potential-specific deviation expected) + self.assertIn(melting_temp, [1008, 1023])
24-32: Consider test performance impact.The test uses large parameters (
number_of_atoms=8000,strain_run_time_steps=1000) that may result in slow execution during CI/CD. If runtime becomes an issue, consider:
- Reducing parameters for faster feedback (e.g., 1000-2000 atoms, 100-500 steps)
- Moving to integration/nightly test suite
- Adding a faster smoke test with minimal parameters
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_melting_lammps.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_melting_lammps.py (1)
src/atomistics/calculators/lammps/melting.py (1)
estimate_melting_temperature(222-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: unittest_old
- GitHub Check: notebooks
- GitHub Check: unittest_qe
- GitHub Check: unittest_siesta
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_grace
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_mace
- GitHub Check: coverage
| 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, | ||
| ) |
There was a problem hiding this comment.
Non-deterministic test: fix the seed parameter.
Passing seed=None triggers random seed generation in estimate_melting_temperature (see melting.py lines 228-229), causing non-deterministic test results. This explains why the assertion accepts two values [1008, 1023], but makes the test flaky and unreliable for CI/CD.
🎲 Proposed fix: use a fixed seed
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])
+ self.assertEqual(melting_temp, 1008) # or 1023, depending on seed=42 resultRun the test with a fixed seed to determine the expected value, then update the assertion accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tests/test_melting_lammps.py around lines 24 - 32, The test is
non-deterministic because estimate_melting_temperature is called with seed=None
which yields random results; update the test call in
tests/test_melting_lammps.py to pass a fixed integer seed (e.g., seed=42) to
estimate_melting_temperature and then re-run the test to capture the
deterministic expected melting_temp value and update the assertion to that
single expected value; reference the estimate_melting_temperature call and the
seed handling in melting.py (around where seed is checked/initialized) when
making the change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.ci_support/environment-lammps.yml (1)
11-11: Minor formatting inconsistency with existing dependencies.The spacing around the
=sign is inconsistent with the existing dependencies (lines 4-10), which use=versionformat without a space before the equals sign.📝 Suggested formatting adjustment
-- pyscal3 = 3.3.0 +- pyscal3 =3.3.0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.ci_support/environment-lammps.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_grace
- GitHub Check: unittest_old
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_qe
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_mace
- GitHub Check: unittest_siesta
- GitHub Check: notebooks
- GitHub Check: coverage
🔇 Additional comments (1)
.ci_support/environment-lammps.yml (1)
11-11: pyscal3 version 3.3.0 is the latest release on conda-forge. No updates are needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/atomistics/calculators/lammps/melting.py`:
- Around line 78-85: The repetition candidates can include zero when r_est < 1
because np.round(r_est) may return 0, causing input_structure.repeat([0,0,0]) to
produce an empty structure; update the construction of candidates/basis_lst so
every repeat factor is at least 1 (e.g., apply max(1, ...) to the round and ceil
results or clip the entire candidates array to >=1) before building basis_lst
and selecting basis, referencing r_est, candidates, basis_lst, basis and
input_structure.repeat.
- Around line 1-6: The import currently brings in the random() function (from
random import random) but the code later calls random.randint(0, 99999) (e.g.,
in functions/methods referencing random.randint), causing AttributeError; change
the import so the module provides randint (either use import random or from
random import randint) and update any references if you choose the latter so
calls to random.randint become just randint; locate the import at the top of the
file and replace it accordingly to fix uses like random.randint.
♻️ Duplicate comments (1)
tests/test_melting_lammps.py (1)
23-35: Non-deterministic test due toseed=None.Passing
seed=Nonetriggers random seed generation inestimate_melting_temperature_using_bisection_CNA(line 406-407 in melting.py), causing non-deterministic test results. The assertion accepting two values[1008, 1023]confirms flakiness.Use a fixed seed for reproducible CI/CD results:
🎲 Proposed fix
melting_temp = estimate_melting_temperature_using_bisection_CNA( structure=input_structure, potential_dataframe=potential_dataframe, target_number_of_atoms=4000, temperature_left=0, temperature_right=1000, temperature_diff_tolerance=10, run=1000, - seed=None, + seed=42, cores=1, log_file=None, ) - self.assertIn(melting_temp, [1008, 1023]) + self.assertEqual(melting_temp, <expected_value>) # Run once with seed=42 to determine
🧹 Nitpick comments (6)
src/atomistics/calculators/lammps/melting.py (6)
19-61: LGTM with optional suggestion.The NPT MD wrapper correctly configures velocity rescaling and xyz coupling. The structure copy-and-update pattern is sound.
Consider replacing
print()statements with Python'sloggingmodule for better control over verbosity in library code, but this is optional for an initial implementation.
138-166: LGTM!The analysis extraction logic is correct. Consider using a
namedtupleor dataclass for the return type to improve readability, but this is optional.
285-286: Potential unreachable case or missing branch.The
elsebranch catches whenstructure_right_dict[key_max] / number_of_atoms > distribution_initial_half > structure_left_dict[key_max] / number_of_atoms(the inverse of line 242-246). This could occur if the initial temperature bounds are inverted or if the structure analysis yields unexpected results.Consider either:
- Adding an explicit branch to handle this case, or
- Improving the error message to aid debugging
Per static analysis (TRY003), consider moving the message to a custom exception class.
📝 Suggested improvement
else: - raise ValueError("We should never reach this point!") + raise ValueError( + f"Unexpected bisection state: left={structure_left_dict[key_max]/number_of_atoms:.3f}, " + f"right={structure_right_dict[key_max]/number_of_atoms:.3f}, threshold={distribution_initial_half:.3f}" + )
332-344: FIXME:temperature_leftassumed to be solid state.The FIXME comment correctly identifies that the algorithm assumes the left bound structure is the optimized (solid) structure without running MD. If a user passes
temperature_left > 0, the assumption may be invalid.Consider either:
- Running MD at
temperature_leftif it's above a threshold (e.g., > 0), or- Documenting this limitation in the docstring
Would you like me to propose a fix that validates the left bound when
temperature_left > 0?
440-440: Unnecessaryint()cast.
round()with a single argument already returns anintin Python 3, making the outerint()call redundant (per static analysis RUF046).♻️ Proposed fix
- return int(round(temperature_final)) + return round(temperature_final)
45-45: Consider usinglogginginstead ofprint()for library code.Multiple
print()calls throughout the module (lines 45, 225-229, 247-251, 267-271, 335, 370) make it difficult for library users to control output verbosity. Consider using theloggingmodule:import logging logger = logging.getLogger(__name__) # Then replace print(...) with logger.info(...)This allows users to configure logging levels and handlers as needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/atomistics/calculators/lammps/melting.pytests/test_melting_lammps.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_melting_lammps.py (1)
src/atomistics/calculators/lammps/melting.py (1)
estimate_melting_temperature_using_bisection_CNA(374-440)
src/atomistics/calculators/lammps/melting.py (1)
src/atomistics/calculators/lammps/libcalculator.py (2)
optimize_positions_and_volume_with_lammpslib(43-83)calc_molecular_dynamics_npt_with_lammpslib(223-310)
🪛 Ruff (0.14.11)
src/atomistics/calculators/lammps/melting.py
286-286: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_qe
- GitHub Check: coverage
- GitHub Check: notebooks
🔇 Additional comments (3)
tests/test_melting_lammps.py (1)
1-11: LGTM on the import guard pattern.The try/except block properly handles the optional LAMMPS dependency, setting
skip_lammps_testappropriately for conditional test execution.src/atomistics/calculators/lammps/melting.py (2)
90-111: LGTM!The diamond detection logic correctly compares "OTHER" counts between CNA and diamond descriptors to determine the appropriate analysis method.
114-135: LGTM!Clean dispatch function for structure analysis.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import operator | ||
| import numpy as np | ||
| from ase import Atoms | ||
| from random import random | ||
| from typing import Optional | ||
| from pandas import DataFrame |
There was a problem hiding this comment.
Critical bug: incorrect import will cause AttributeError.
Line 4 imports the random() function, but line 407 calls random.randint(0, 99999). The function random doesn't have a .randint attribute—this will raise AttributeError: 'builtin_function_or_method' object has no attribute 'randint'.
🐛 Proposed fix
import operator
import numpy as np
from ase import Atoms
-from random import random
+import random
from typing import Optional
from pandas import DataFrame📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import operator | |
| import numpy as np | |
| from ase import Atoms | |
| from random import random | |
| from typing import Optional | |
| from pandas import DataFrame | |
| import operator | |
| import numpy as np | |
| from ase import Atoms | |
| import random | |
| from typing import Optional | |
| from pandas import DataFrame |
🤖 Prompt for AI Agents
In `@src/atomistics/calculators/lammps/melting.py` around lines 1 - 6, The import
currently brings in the random() function (from random import random) but the
code later calls random.randint(0, 99999) (e.g., in functions/methods
referencing random.randint), causing AttributeError; change the import so the
module provides randint (either use import random or from random import randint)
and update any references if you choose the latter so calls to random.randint
become just randint; locate the import at the top of the file and replace it
accordingly to fix uses like random.randint.
| r_est = (target_number_of_atoms / len(input_structure)) ** (1 / 3) | ||
| candidates = np.array( | ||
| [max(1, int(np.floor(r_est))), int(np.round(r_est)), int(np.ceil(r_est))] | ||
| ) | ||
| basis_lst = [input_structure.repeat([i, i, i]) for i in candidates] | ||
| basis = basis_lst[ | ||
| np.argmin([np.abs(len(b) - target_number_of_atoms) for b in basis_lst]) | ||
| ] |
There was a problem hiding this comment.
Edge case: potential for zero repetition factor.
When target_number_of_atoms < len(input_structure), r_est < 1, and np.round(r_est) could yield 0 (e.g., np.round(0.3) = 0), causing repeat([0, 0, 0]) to create an empty structure.
The max(1, ...) guard only applies to the floor value, not to round:
🛡️ Proposed defensive fix
r_est = (target_number_of_atoms / len(input_structure)) ** (1 / 3)
candidates = np.array(
- [max(1, int(np.floor(r_est))), int(np.round(r_est)), int(np.ceil(r_est))]
+ [max(1, int(np.floor(r_est))), max(1, int(np.round(r_est))), max(1, int(np.ceil(r_est)))]
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r_est = (target_number_of_atoms / len(input_structure)) ** (1 / 3) | |
| candidates = np.array( | |
| [max(1, int(np.floor(r_est))), int(np.round(r_est)), int(np.ceil(r_est))] | |
| ) | |
| basis_lst = [input_structure.repeat([i, i, i]) for i in candidates] | |
| basis = basis_lst[ | |
| np.argmin([np.abs(len(b) - target_number_of_atoms) for b in basis_lst]) | |
| ] | |
| r_est = (target_number_of_atoms / len(input_structure)) ** (1 / 3) | |
| candidates = np.array( | |
| [max(1, int(np.floor(r_est))), max(1, int(np.round(r_est))), max(1, int(np.ceil(r_est)))] | |
| ) | |
| basis_lst = [input_structure.repeat([i, i, i]) for i in candidates] | |
| basis = basis_lst[ | |
| np.argmin([np.abs(len(b) - target_number_of_atoms) for b in basis_lst]) | |
| ] |
🤖 Prompt for AI Agents
In `@src/atomistics/calculators/lammps/melting.py` around lines 78 - 85, The
repetition candidates can include zero when r_est < 1 because np.round(r_est)
may return 0, causing input_structure.repeat([0,0,0]) to produce an empty
structure; update the construction of candidates/basis_lst so every repeat
factor is at least 1 (e.g., apply max(1, ...) to the round and ceil results or
clip the entire candidates array to >=1) before building basis_lst and selecting
basis, referencing r_est, candidates, basis_lst, basis and
input_structure.repeat.
|
@prabhath-c there still seems to be an issue with the imports: |
| print( | ||
| f"Both structures have {key_max} above half distribution at temperatures {temperature_left} and {temperature_right}" | ||
| ) | ||
| print("Increasing temperatures...") | ||
| print("-----------------------------------------------------") |
There was a problem hiding this comment.
The print messages should be optional, maybe add a debug flag to enable and disable the print statements.
| print( | ||
| f"Left structure has {key_max} above half distribution at temperature {temperature_left}, right structure below at temperature {temperature_right}" | ||
| ) | ||
| print("Decreasing temperatures...") | ||
| print("-----------------------------------------------------") |
There was a problem hiding this comment.
Add a debug option for print statements
| print( | ||
| f"Both structures have {key_max} below half distribution at temperatures {temperature_left} and {temperature_right}" | ||
| ) | ||
| print("Decreasing temperatures...") | ||
| print("-----------------------------------------------------") |
There was a problem hiding this comment.
Add a debug option for print statements
| from ase.data import reference_states, atomic_numbers | ||
| import numpy as np | ||
| from ase import Atoms | ||
| from random import random |
There was a problem hiding this comment.
| from random import random | |
| import random |
| Atoms: The structure after MD simulation. | ||
| """ | ||
|
|
||
| print(f"Running NPT MD at T = {temperature} K") |
| structure (pyiron_atomistics.structure.atoms.Atoms): The structure to analyze. | ||
| mode ("total"/"numeric"/"str"): Controls the style and level | ||
| of detail of the output. | ||
| - total : return number of atoms belonging to each structure | ||
| - numeric : return a per atom list of numbers- 0 for unknown, | ||
| 1 fcc, 2 hcp, 3 bcc and 4 icosa | ||
| - str : return a per atom string of sructures | ||
| diamond (bool): Flag to either use the diamond structure detector or | ||
| the common neighbor analysis. | ||
| structure (Atoms): The structure to analyse. |
There was a problem hiding this comment.
Why was the docstring removed ?
| ) | ||
| temperature_step = temperature_right - temperature_left | ||
|
|
||
| print(f"Melting point estimated at T = {temperature_left} K") |
This pull request includes the changes in:
Example:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.