Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a new VASPModel dataclass (fields: xc, hfscreen) and exports it from models and the top-level package; replaces rdkit2ase.pack with molify.pack.pack in Packmol/MultiPackmol, adds a configurable ratio parameter, and introduces tests for Packmol behavior and ratio propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🤖 Fix all issues with AI agents
In `@ipsuite/configuration_generation/packmol.py`:
- Line 56: The Packmol class currently declares ratio but never uses it in
Packmol.run (which calls rdkit2ase.pack), so either remove ratio from Packmol
and declare it only on MultiPackmol, or make Packmol.run call molify.pack and
pass the ratio through; also add ratio to Packmol's class docstring Attributes.
Concretely: update either the attribute location (move ratio from Packmol to
MultiPackmol and remove unused import/usages), or change Packmol.run to call
molify.pack(..., ratio=self.ratio) instead of rdkit2ase.pack, ensure
MultiPackmol.run still works, and add the ratio entry to Packmol's docstring
Attributes.
In `@ipsuite/models/vasp.py`:
- Around line 9-13: Replace the placeholder docstring for the VASPModel class
with a concise NumPy-style docstring that explains the purpose and behavior of
the Node (class VASPModel inheriting NodeWithCalculator), lists any important
attributes or parameters the Node expects, and shows a runnable/testable example
using project and ips (e.g., creating a project, instantiating VASPModel,
running it via ips). Ensure the docstring contains a short one-line summary, a
"Parameters" (if applicable) and "Examples" section demonstrating usage with
project and ips so automated tests can import and execute the snippet.
- Around line 19-23: The get_calculator method currently ignores instance
fields, doesn't pass directory, has dead code, and mismatches the
NodeWithCalculator signature; update get_calculator to accept directory: str |
Path | None = None (make it keyword-optional to match the Protocol), construct
and return Vasp using self.xc and self.hfscreen, and pass the directory via the
appropriate Vasp keyword (e.g., directory=directory or set the working directory
arg) along with **kwargs; remove the unreachable raise NotImplementedError() and
ensure the function signature and behavior align with the NodeWithCalculator
Protocol and any Vasp constructor expectations.
🧹 Nitpick comments (2)
ipsuite/configuration_generation/packmol.py (1)
134-137: Remove commented-out debug prints.These leftover debug statements should be cleaned up before merging.
Proposed fix
def run(self): - # print("RATIO IN NODE:", self.ratio) - # print("PACK FUNCTION:", rdkit2ase.pack) - # print("MODULE:", rdkit2ase.pack.__module__) - # print(rdkit2ase.__file__) np.random.seed(self.seed)ipsuite/models/vasp.py (1)
17-17: Remove commented-out code.The commented-out
configfield is dead code. If planned for later, track it in an issue instead.
ipsuite/models/vasp.py
Outdated
Comment on lines
+9
to
+13
| @dataclasses.dataclass | ||
| class VASPModel(NodeWithCalculator): | ||
| """ | ||
| Docstring for VASPModel | ||
| """ |
There was a problem hiding this comment.
Placeholder docstring must be replaced.
The coding guidelines require each Node to have a concise docstring describing its functionality, written in NumPy style, including a testable example using project and ips. "Docstring for VASPModel" does not meet any of these requirements. As per coding guidelines: "Each Node must have a docstring describing its functionality" and "Each Node's docstring should include a testable example using project and ips".
🤖 Prompt for AI Agents
In `@ipsuite/models/vasp.py` around lines 9 - 13, Replace the placeholder
docstring for the VASPModel class with a concise NumPy-style docstring that
explains the purpose and behavior of the Node (class VASPModel inheriting
NodeWithCalculator), lists any important attributes or parameters the Node
expects, and shows a runnable/testable example using project and ips (e.g.,
creating a project, instantiating VASPModel, running it via ips). Ensure the
docstring contains a short one-line summary, a "Parameters" (if applicable) and
"Examples" section demonstrating usage with project and ips so automated tests
can import and execute the snippet.
ipsuite/models/vasp.py
Outdated
Comment on lines
+19
to
+23
| def get_calculator(self, directory: str | Path, **kwargs) -> Calculator: | ||
| from ase.calculators.vasp import Vasp | ||
|
|
||
| return Vasp(xc="hse06", hfscreen=0.4) | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Multiple issues in get_calculator.
- Hardcoded values instead of instance fields —
self.xcandself.hfscreenare ignored; the constructor always gets"hse06"and0.4. directoryis never forwarded to theVaspcalculator, so it will default to writing in the current directory.- Unreachable code —
raise NotImplementedError()on line 23 is dead code after thereturn. - Signature mismatch with
NodeWithCalculatorProtocol — the protocol declaresdirectory: str | pathlib.Path | None = None(keyword-only via*), but heredirectoryis positional and non-optional.
Proposed fix
- def get_calculator(self, directory: str | Path, **kwargs) -> Calculator:
+ def get_calculator(self, *, directory: str | Path | None = None, **kwargs) -> Calculator:
from ase.calculators.vasp import Vasp
- return Vasp(xc="hse06", hfscreen=0.4)
- raise NotImplementedError()
+ return Vasp(xc=self.xc, hfscreen=self.hfscreen, directory=directory)
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 19-19: Unused method argument: directory
(ARG002)
[warning] 19-19: Unused method argument: kwargs
(ARG002)
🤖 Prompt for AI Agents
In `@ipsuite/models/vasp.py` around lines 19 - 23, The get_calculator method
currently ignores instance fields, doesn't pass directory, has dead code, and
mismatches the NodeWithCalculator signature; update get_calculator to accept
directory: str | Path | None = None (make it keyword-optional to match the
Protocol), construct and return Vasp using self.xc and self.hfscreen, and pass
the directory via the appropriate Vasp keyword (e.g., directory=directory or set
the working directory arg) along with **kwargs; remove the unreachable raise
NotImplementedError() and ensure the function signature and behavior align with
the NodeWithCalculator Protocol and any Vasp constructor expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test_pack.py`:
- Around line 73-90: The test patches target the wrong module; update the patch
decorators in test_packmol_run_calls_pack to patch the names where Packmol looks
them up (pack and znh5md.IO) in ipsuite.configuration_generation.packmol rather
than ipsuite.pack; specifically change the patch targets to
"ipsuite.configuration_generation.packmol.pack" and
"ipsuite.configuration_generation.packmol.znh5md" (and adapt the test signature
to mock_znh5md, mock_pack), then set mock_pack.return_value = dummy_atoms() and
mock_znh5md.IO.return_value = MagicMock() so Packmol.run() uses the injected
mocks.
Comment on lines
+73
to
+90
| @patch("ipsuite.pack.pack") | ||
| @patch("ipsuite.pack.znh5md.IO") | ||
| def test_packmol_run_calls_pack(mock_io, mock_pack): | ||
| mock_pack.return_value = dummy_atoms() | ||
| mock_io.return_value = MagicMock() | ||
|
|
||
| node = Packmol( | ||
| data=[[dummy_atoms()]], | ||
| count=[1], | ||
| density=1000, | ||
| ratio=[1.0, 2.0, 1.0], | ||
| ) | ||
|
|
||
| node.run() | ||
|
|
||
| mock_pack.assert_called_once() | ||
| kwargs = mock_pack.call_args.kwargs | ||
| assert kwargs["ratio"] == [1.0, 2.0, 1.0] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how pack is imported and used in the production packmol module
echo "=== Finding packmol.py ==="
fd packmol.py --type f
echo ""
echo "=== Contents of packmol.py ==="
fd packmol.py --type f -exec cat -n {} \;
echo ""
echo "=== Checking ipsuite/pack directory structure ==="
find ipsuite/pack -type f -name "*.py" | head -20
echo ""
echo "=== Contents of ipsuite/pack/__init__.py ==="
if [ -f "ipsuite/pack/__init__.py" ]; then
cat -n ipsuite/pack/__init__.py
else
echo "File not found"
fi
echo ""
echo "=== Check if molify.pack exists ==="
fd "pack" ipsuite --type f --name "*.py" | rg molify
echo ""
echo "=== Checking test_pack.py to see exact imports and line context ==="
fd test_pack.py --type f -exec cat -n {} \;
Repository: zincware/IPSuite
Length of output: 1158
🏁 Script executed:
#!/bin/bash
echo "=== Contents of packmol.py ==="
cat ipsuite/configuration_generation/packmol.py
echo ""
echo "=== Directory structure under ipsuite ==="
ls -la ipsuite/
echo ""
echo "=== Looking for test_pack.py ==="
find . -name "test_pack.py" -type f
echo ""
echo "=== Contents of test_pack.py ==="
find . -name "test_pack.py" -type f -exec cat {} \;
echo ""
echo "=== Checking imports in packmol.py for 'pack' ==="
grep -n "import\|from" ipsuite/configuration_generation/packmol.py | head -20
Repository: zincware/IPSuite
Length of output: 9440
Correct the patch targets to reference the module where pack and znh5md.IO are actually imported.
The production code in ipsuite/configuration_generation/packmol.py imports pack via from molify.pack import pack (line 15). The test patches target ipsuite.pack.pack and ipsuite.pack.znh5md.IO, but ipsuite.pack is not an actual module in the codebase. Mocks must patch where the names are looked up, not where they're defined. Update the patches to:
`@patch`("ipsuite.configuration_generation.packmol.pack")
`@patch`("ipsuite.configuration_generation.packmol.znh5md")
def test_packmol_run_calls_pack(mock_znh5md, mock_pack):
mock_pack.return_value = dummy_atoms()
mock_znh5md.IO.return_value = MagicMock()
# ... rest of test
🤖 Prompt for AI Agents
In `@test_pack.py` around lines 73 - 90, The test patches target the wrong module;
update the patch decorators in test_packmol_run_calls_pack to patch the names
where Packmol looks them up (pack and znh5md.IO) in
ipsuite.configuration_generation.packmol rather than ipsuite.pack; specifically
change the patch targets to "ipsuite.configuration_generation.packmol.pack" and
"ipsuite.configuration_generation.packmol.znh5md" (and adapt the test signature
to mock_znh5md, mock_pack), then set mock_pack.return_value = dummy_atoms() and
mock_znh5md.IO.return_value = MagicMock() so Packmol.run() uses the injected
mocks.
PythonFZ
requested changes
Feb 16, 2026
Member
PythonFZ
left a comment
PythonFZ
left a comment
There was a problem hiding this comment.
The Packmol changes look good, but there are few things to consider:
uv add molifythe package is not a dependency of ipsuite yet- the tests need to be rewritten
- the vasp calculator should be handled in a different PR and removed from this one, e.g. git checkout the related files from main
Comment on lines
+135
to
+138
| # print("RATIO IN NODE:", self.ratio) | ||
| # print("PACK FUNCTION:", rdkit2ase.pack) | ||
| # print("MODULE:", rdkit2ase.pack.__module__) | ||
| # print(rdkit2ase.__file__) |
|
|
||
| frames = [ | ||
| rdkit2ase.pack( | ||
| # rdkit2ase.pack( |
|
|
||
| frames.append( | ||
| rdkit2ase.pack( | ||
| # rdkit2ase.pack( |
Comment on lines
+1
to
+4
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import ase | ||
| import pytest |
Member
There was a problem hiding this comment.
In IPSuite integration tests should use the testing procedure a user would use.
See https://github.com/zincware/IPSuite/blob/main/tests/integration/test_smiles_to_atoms.py
volneu
force-pushed
the
feat/multipackmol_change-box-shape
branch
from
9953162 to
8e19bff
Compare
for more information, see https://pre-commit.ci
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.
One can use the keyword "ratio=[float,float,float]" under ips.MultiPackmol to reshape the simulation box, and the "Multipackmol" will call "pack" from molify.pack, instead of "rdkit2ase.pack".
Summary by CodeRabbit
New Features
Tests