Fix __init__.py and __init__.pyi #446
Conversation
…graph model, no duplicates in FilterAllowedMolecules
…ained, assumed 1 bin edge before
for more information, see https://pre-commit.ci
…dded log_file out to ASEGeoOpt node
|
I'm sorry, it looks like this branch also has the cutoff pull request (#444), I do not understand how to sync to the |
… for __init__.py and __init__.pyi
TobiasDijkhuis
force-pushed
the
fix_init
branch
from
1b81fa6 to
2eaefd2
Compare
|
What is the purpose of |
|
merged #445 - you might want to |
|
Could you add the |
📝 WalkthroughWalkthroughThe pull request comprises documentation typo fixes, resource management improvements via matplotlib figure closure, public API restructuring in package initialization files, and feature additions for customizable atomic cutoff radii across geometry and analysis modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 2
🤖 Fix all issues with AI agents
In `@ipsuite/geometry/graphs.py`:
- Around line 40-41: Update the docstring return type for c_list to match the
function signature and actual return value: change "c_list (np.ndarray)" to
"c_list (list[np.ndarray])" or a clear description like "list of numpy.ndarray"
so the documented type matches the returned list of arrays; keep the variable
name c_list as referenced in the docstring.
- Around line 7-26: The docstring for atoms_to_graph incorrectly says "cutoffs
of each atom" while the parameter is per-element cutoffs keyed by element
symbol; update the cutoffs description in atoms_to_graph to match
identify_molecules (e.g., "cutoffs of each element (dict keyed by element
symbol, values are cutoff radii)"), ensuring the wording clarifies keys are
element symbols and default behavior uses ase.data.covalent_radii.
🧹 Nitpick comments (8)
ipsuite/bootstrap/surface_mods.py (1)
178-178: Typo in function name:plot_ture_vs_pred→plot_true_vs_pred.Suggested fix
-def plot_ture_vs_pred(x, y, z, name, height, plots_dir): +def plot_true_vs_pred(x, y, z, name, height, plots_dir):Also update the call sites in
SurfaceRasterMetrics.get_plots(lines 160 and 168).ipsuite/analysis/forces.py (1)
99-99: LGTM! Resource cleanup is appropriate.For seaborn jointplots, consider using
plt.close(joint_plot.figure)to explicitly close the correct figure, asjointplotmanages its own figure object.More explicit alternative
plt.tight_layout() plt.savefig(self.figure_path) - plt.close() + plt.close(joint_plot.figure)ipsuite/configuration_selection/threshold.py (1)
142-143: Consider closing the specific figure explicitly.Using
plt.close(fig)instead ofplt.close()ensures the correct figure is closed, avoiding potential issues if the active figure state changes unexpectedly.Suggested improvement
fig.savefig(self.img_selection, bbox_inches="tight") - plt.close() + plt.close(fig)ipsuite/configuration_selection/base.py (1)
90-91: Consider closing the specific figure explicitly.Same suggestion as in
threshold.py— useplt.close(fig)to explicitly close the created figure.Suggested improvement
fig.savefig(self.img_selection, bbox_inches="tight") - plt.close() + plt.close(fig)ipsuite/configuration_selection/filter.py (1)
62-63: Consider closing the specific figure explicitly.For consistency with the other suggestions, use
plt.close(fig)here as well.Suggested improvement
fig.savefig(self.histogram, bbox_inches="tight") - plt.close() + plt.close(fig)ipsuite/__init__.py (1)
23-58:__all__redefinition is intentional but worth noting.Ruff flags
__all__as redefined (line 25 shadows line 23 fromlazy_loader.attach_stub). This is a known pattern from mkinit's--lazy_loader_typedoutput where the stub-generated__all__is overwritten with an explicit list for static analysis tools.If Ruff continues to flag this, consider adding a
# noqa: F811comment on line 25 to suppress the warning, or configure Ruff to ignore this specific instance inpyproject.toml. Since this file is auto-generated, maintaining the mkinit output verbatim is reasonable.Regarding the RUF022 (unsorted
__all__) hint: sorting would break round-trip regeneration with mkinit unless mkinit itself sorts the output. You may safely ignore this lint or disable RUF022 for this file.ipsuite/__init__.pyi (2)
128-147: Duplicate submodule imports detected.Several submodule imports are duplicated:
baseis imported on both line 1 and line 129This appears to be from mkinit generating imports that were already manually present in the file. While not harmful at runtime, it creates maintenance overhead.
Consider either:
- Removing the manual imports (lines 1-127) and relying solely on mkinit-generated content, or
- Running mkinit on a clean file without pre-existing imports
This would produce a cleaner, more maintainable stub file.
149-193: Duplicate symbol imports from submodules.Many symbols are imported twice—once in the original manual section (lines 4-125) and again in the mkinit-generated section (lines 149-193). For example:
AllowedStructuresFilterappears on lines 5 and 149Flattenappears on lines 31 and 160ApplyCalculatorappears on lines 41 and 164Additionally, line 161 imports
interfacesfrom.base, which shadows theinterfacessubmodule imported on line 141.Since
.pyifiles are for static type checkers, these duplicates won't cause runtime issues, but they add noise and may cause confusion during maintenance. A cleaner approach would be to regenerate the file from scratch with mkinit rather than appending to existing content.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/source/_get_started/ips.rstdocs/source/index.rstipsuite/__init__.pyipsuite/__init__.pyiipsuite/analysis/bond_stretch.pyipsuite/analysis/forces.pyipsuite/analysis/md.pyipsuite/analysis/molecules.pyipsuite/analysis/sensitivity.pyipsuite/bootstrap/surface_mods.pyipsuite/calculators/ase_geoopt.pyipsuite/configuration_selection/base.pyipsuite/configuration_selection/filter.pyipsuite/configuration_selection/threshold.pyipsuite/geometry/graphs.pyipsuite/geometry/mapping.pypyproject.toml
🧰 Additional context used
📓 Path-based instructions (2)
ipsuite/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
ipsuite/**/*.py: Use full type annotations throughout the IPSuite source code
Write docstrings in NumPy style
Each Node must have a docstring describing its functionality
Docstrings must be concise and clear
Docstrings should not include unnecessary background (e.g., do not explain domain basics such as what molecular dynamics is)
Each Node’s docstring should include a testable example usingprojectandips
All connections between nodes must use full type annotations
Files:
ipsuite/analysis/bond_stretch.pyipsuite/bootstrap/surface_mods.pyipsuite/configuration_selection/filter.pyipsuite/analysis/molecules.pyipsuite/configuration_selection/threshold.pyipsuite/configuration_selection/base.pyipsuite/analysis/sensitivity.pyipsuite/analysis/md.pyipsuite/geometry/mapping.pyipsuite/calculators/ase_geoopt.pyipsuite/geometry/graphs.pyipsuite/__init__.pyipsuite/analysis/forces.py
ipsuite/__init__.pyi
📄 CodeRabbit inference engine (AGENTS.md)
Expose and manage public imports via lazy-loader in
ipsuite/__init__.pyi
Files:
ipsuite/__init__.pyi
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: zincware/IPSuite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:01.158Z
Learning: Applies to ipsuite/**/*.py : Use full type annotations throughout the IPSuite source code
📚 Learning: 2025-09-02T16:50:01.158Z
Learnt from: CR
Repo: zincware/IPSuite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:01.158Z
Learning: Applies to ipsuite/__init__.pyi : Expose and manage public imports via lazy-loader in `ipsuite/__init__.pyi`
Applied to files:
ipsuite/__init__.pyiipsuite/__init__.py
📚 Learning: 2025-09-02T16:50:01.158Z
Learnt from: CR
Repo: zincware/IPSuite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:01.158Z
Learning: Applies to ipsuite/**/*.py : Use full type annotations throughout the IPSuite source code
Applied to files:
ipsuite/__init__.pyiipsuite/__init__.py
📚 Learning: 2025-09-02T16:50:01.158Z
Learnt from: CR
Repo: zincware/IPSuite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:01.158Z
Learning: Applies to ipsuite/**/*.py : Each Node’s docstring should include a testable example using `project` and `ips`
Applied to files:
ipsuite/__init__.py
📚 Learning: 2025-09-02T16:50:01.158Z
Learnt from: CR
Repo: zincware/IPSuite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:01.158Z
Learning: Applies to ipsuite/**/*.py : Each Node must have a docstring describing its functionality
Applied to files:
ipsuite/__init__.py
🧬 Code graph analysis (3)
ipsuite/analysis/molecules.py (1)
ipsuite/geometry/mapping.py (1)
BarycenterMapping(12-55)
ipsuite/geometry/mapping.py (1)
ipsuite/geometry/graphs.py (1)
identify_molecules(29-46)
ipsuite/__init__.py (2)
ipsuite/utils/helpers.py (1)
setup_ase(11-24)ipsuite/utils/logs.py (1)
setup_logging(7-22)
🪛 Ruff (0.14.11)
ipsuite/__init__.py
25-25: Redefinition of unused __all__ from line 23: __all__ redefined here
(F811)
25-58: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ 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). (1)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (18)
ipsuite/calculators/ase_geoopt.py (1)
82-82: LGTM! Consider addinglog_fileto the Attributes docstring.The new
log_fileattribute is correctly typed and properly passed to the ASE optimizer. For documentation consistency, consider adding it to the Attributes section in the docstring alongsidetraj_file.Also applies to: 109-109
ipsuite/bootstrap/surface_mods.py (1)
199-199: LGTM! Good resource cleanup.The
plt.close()correctly releases figure resources after saving. For consistency with other files in this PR (e.g.,bond_stretch.py), consider usingplt.close(fig)to explicitly close the specific figure.ipsuite/analysis/bond_stretch.py (1)
107-108: LGTM! Proper figure cleanup for both plots.The explicit closing of both
e_figandf_figis correct. Minor style note:plt.close(e_fig)(positional) is more idiomatic thanplt.close(fig=e_fig)(keyword), though both work.ipsuite/analysis/md.py (1)
33-33: LGTM! Good resource management.For consistency with
bond_stretch.pywhere explicit figure references are used, considerplt.close(fig).docs/source/_get_started/ips.rst (1)
29-29: LGTM!Good catch on fixing the typo in the alt text ("Develepment" → "Development").
pyproject.toml (1)
87-87: LGTM!The Ruff configuration adjustments correctly accommodate the mkinit-generated files:
- Excluding
.pyifrom the general exclude list (stub files need different handling).- Ignoring E402, F811, and I001 for the init files aligns with the lazy-loader pattern used by mkinit.
Also applies to: 102-104
ipsuite/analysis/sensitivity.py (2)
83-94: Good resource management with explicit figure cleanup.The
plt.close()calls after saving figures prevent memory leaks when generating multiple plots. This is a sensible improvement.A minor refinement would be
plt.close(fig)to explicitly close the specific figure, but the current approach works correctly since each figure is saved immediately before closing.
153-164: LGTM — consistent resource cleanup.The
plt.close()addition follows the same pattern established inAnalyseGlobalForceSensitivity.run, ensuring the sensitivity plot figure is properly released.docs/source/index.rst (1)
20-20: Good catch — typo fix for toctree reference.Correcting
modlules→modulesensures the documentation link resolves correctly.ipsuite/__init__.py (1)
3-10: Good addition of mkinit documentation.This addresses the reviewer's request to document the regeneration command, making it discoverable for future maintenance.
ipsuite/__init__.pyi (1)
195-228:__all__list looks comprehensive.The exported symbols cover submodules and public classes appropriately. This enables both
ips.Projectandips.project.Projectimport paths to work with static type checkers, which aligns with the PR objectives.ipsuite/analysis/molecules.py (2)
24-32: Well-documentedcutoffsparameter with proper type annotation.The new
cutoffsattribute follows the established pattern with:
- Full type annotation (
dict[str, float] | None)- Clear docstring documentation
- Sensible default (
Nonedefers toase.data.covalent_radii)This aligns with the coding guidelines requiring full type annotations.
37-54: LGTM —cutoffscorrectly propagated toBarycenterMapping.The
cutoffsparameter is properly passed to theBarycenterMappingconstructor, which then forwards it tographs.identify_moleculesas shown in the relevant code snippet fromipsuite/geometry/mapping.py.ipsuite/geometry/mapping.py (2)
28-35: LGTM!The new
cutoffsattribute is well-documented with a NumPy-style docstring and has proper type annotations. The default value ofNonecorrectly defers toase.data.covalent_radiias documented.
41-41: LGTM!The
cutoffsparameter is correctly propagated tographs.identify_molecules, enabling per-element cutoff customization during molecule identification.ipsuite/geometry/graphs.py (3)
4-4: LGTM!The
natural_cutoffsimport is correctly added to support per-element cutoff customization.
21-23: LGTM!The
natural_cutoffsusage is correct. Whencutoffsdict is provided (e.g.,{'H': 1.0, 'O': 1.5}), it's unpacked as keyword arguments tonatural_cutoffs, which returns per-atom cutoff values that override the default covalent radii for the specified elements.
29-46: LGTM on the function implementation!The
cutoffsparameter is correctly propagated toatoms_to_graph, and the function properly identifies molecules based on connected subgraphs with customizable cutoff radii.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def atoms_to_graph(atoms: ase.Atoms, cutoffs: dict[str, float] | None = None) -> nx.Graph: | ||
| """Converts ASE Atoms into a Graph based on their | ||
| bond connectivity. | ||
| Args: | ||
| atoms (ase.Atoms): Atoms instance to convert | ||
| cutoffs (dict[str, float] | None): cutoffs of each atom. | ||
| Dictionary with keys for the symbols and values of the cutoff radii. | ||
| If None, use the `ase.data.covalent_radii`. Default: None | ||
| Returns: | ||
| G (nx.Graph): Connectivity graph | ||
| """ | ||
| # This can be optimized by reusing the NL! | ||
| nl = build_neighbor_list(atoms, self_interaction=False) | ||
| if cutoffs is not None: | ||
| cutoffs = natural_cutoffs(atoms, **cutoffs) | ||
| nl = build_neighbor_list(atoms, self_interaction=False, cutoffs=cutoffs) | ||
| cm = nl.get_connectivity_matrix(sparse=False) | ||
| G = nx.from_numpy_array(cm) | ||
| return G |
There was a problem hiding this comment.
Minor docstring inconsistency: "atom" vs "element".
Line 13 describes cutoffs as "cutoffs of each atom" but the parameter represents per-element cutoffs (keyed by element symbol). The identify_molecules docstring at line 36 correctly says "cutoffs of each element."
Proposed fix
Args:
atoms (ase.Atoms): Atoms instance to convert
- cutoffs (dict[str, float] | None): cutoffs of each atom.
+ cutoffs (dict[str, float] | None): cutoffs of each element.
Dictionary with keys for the symbols and values of the cutoff radii.
If None, use the `ase.data.covalent_radii`. Default: None🤖 Prompt for AI Agents
In `@ipsuite/geometry/graphs.py` around lines 7 - 26, The docstring for
atoms_to_graph incorrectly says "cutoffs of each atom" while the parameter is
per-element cutoffs keyed by element symbol; update the cutoffs description in
atoms_to_graph to match identify_molecules (e.g., "cutoffs of each element (dict
keyed by element symbol, values are cutoff radii)"), ensuring the wording
clarifies keys are element symbols and default behavior uses
ase.data.covalent_radii.
| Returns: | ||
| c_list (np.ndarray): Array of lists of connected atom indices |
There was a problem hiding this comment.
Minor docstring inconsistency: return type.
The docstring states c_list (np.ndarray) but the function signature and actual return value is list[np.ndarray].
Proposed fix
Returns:
- c_list (np.ndarray): Array of lists of connected atom indices
+ c_list (list[np.ndarray]): List of arrays of connected atom indices📝 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.
| Returns: | |
| c_list (np.ndarray): Array of lists of connected atom indices | |
| Returns: | |
| c_list (list[np.ndarray]): List of arrays of connected atom indices |
🤖 Prompt for AI Agents
In `@ipsuite/geometry/graphs.py` around lines 40 - 41, Update the docstring return
type for c_list to match the function signature and actual return value: change
"c_list (np.ndarray)" to "c_list (list[np.ndarray])" or a clear description like
"list of numpy.ndarray" so the documented type matches the returned list of
arrays; keep the variable name c_list as referenced in the docstring.
2 participants
Fixed
__init__.pyand__init__.pyisuch that it works better for static type hinters. Now, my static type checker can actually determine the types of ips.project.Project, and both ips.Project and ips.project.Project work when imported.See mkinit, and the network issue about this networkx issue.
I generated the files by running
mkinit --relative --lazy_loader_typed -win theipsuitedirectory. For some reason the__all__in__init__.pyidid not have a closing bracket, so I added that manually.Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.