feat: support customize cutoff-based detection by ase

[SchrodingersCattt]

Summary by CodeRabbit

• New Features

  • ASE-based bond detection with per‑pair custom cutoffs and a global cutoff multiplier; avoids duplicate bonds across periodic boundaries.
  • Optional SciPy-powered connected‑components grouping for bond clustering.

• Improvements

  • Auto-enables ASE mode when custom cutoffs or a non‑default multiplier are supplied (with a warning).
  • New CLI flags: --use-ase, --ase-cutoff-mult, --ase-pair-cutoffs.
  • Improved logging during LAMMPS file processing.

• Tests

  • Expanded coverage for ASE mode, CLI args, cutoff parsing, bond detection, deduplication, and clustering.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds ASE-based bond detection with configurable global and per-pair cutoffs, optional SciPy connected-component grouping for bonds, CLI flags to control ASE behavior, auto-enable logic for ASE when cutoff parameters are provided, extra logging, and new tests for these flows.

Changes

Cohort / File(s)	Summary
ASE bond detection & helpers reacnetgenerator/_detect.py	Adds _getbondfromase() (ASE neighbor-list bond perception with global multiplier and parsed per-pair cutoffs), _parse_custom_cutoffs(); _getbondfromcrd() delegates to ASE when enabled; _connectmolecule() may use SciPy connected-components; adds LAMMPS-reading logging.
CLI option exposure reacnetgenerator/commandline.py	Adds CLI flags --use-ase, --ase-cutoff-mult, --ase-pair-cutoffs; passes ASE options into runtime and includes flags in parm2cmd().
ReacNetGenerator config / auto-enable reacnetgenerator/reacnetgen.py	Adds default config keys use_ase, ase_cutoff_mult, custom_cutoffs; auto-enables use_ase (with a warning) when custom cutoffs or non-default multiplier provided.
Unit tests — ASE mode & CLI tests/test_ase.py, tests/test_ase_cli.py, tests/test_detect.py, tests/test_reacnetgen.py	Adds tests for ASE parsing, _getbondfromase() behavior (bonds, levels, PBC deduplication), SciPy clustering branch, CLI parsing/help, and auto-enable logic.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as commandline.py
    participant RNG as ReacNetGenerator
    participant Detect as _detect.py
    participant ASE as ASE
    participant SciPy as SciPy

    User->>CLI: invoke with ASE flags or custom cutoffs
    CLI->>RNG: pass use_ase, ase_cutoff_mult, custom_cutoffs
    RNG->>RNG: evaluate auto-enable and set use_ase if needed
    alt use_ase == true
        RNG->>Detect: request bond detection (_getbondfromcrd)
        Detect->>Detect: _parse_custom_cutoffs()
        Detect->>ASE: neighbor-list using global_cutoff*mult + per-pair cutoffs
        ASE-->>Detect: neighbor pairs (deduplicated across PBC)
        Detect->>Detect: assemble per-atom bonds and bond levels
    else
        RNG->>Detect: request bond detection (OpenBabel path)
    end
    Detect->>Detect: _connectmolecule(bonds)
    alt SciPy available
        Detect->>SciPy: build CSR graph, compute connected components
        SciPy-->>Detect: component labels
        Detect->>Detect: split bonds by component
    else
        Detect->>Detect: fallback DPS-based grouping
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: adding support for customizable cutoff-based bond detection using ASE (Atomic Simulation Environment).
Docstring Coverage	✅ Passed	Docstring coverage is 89.80% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 94.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.23%. Comparing base (a7c28d9) to head (2a49d35).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
reacnetgenerator/commandline.py	66.66%	3 Missing ⚠️
reacnetgenerator/_detect.py	98.41%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2408      +/-   ##
==========================================
- Coverage   95.26%   95.23%   -0.03%     
==========================================
  Files          17       17              
  Lines        1457     1532      +75     
==========================================
+ Hits         1388     1459      +71     
- Misses         69       73       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

Merging this PR will improve performance by 17.55%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 7 untouched benchmarks
⏩ 8 skipped benchmarks¹

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
⚡	WallTime	test_benchmark_hmm[reacnetgen_param2]	2.2 ms	1.8 ms	+17.18%
⚡	WallTime	test_benchmark_hmm[reacnetgen_param1]	2.2 ms	1.8 ms	+17.55%

---

_{Comparing SchrodingersCattt:feat/support-ase-detect (2a49d35) with master (a7c28d9)}

Footnotes

1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

reacnetgenerator/_detect.py (1)

221-221: Prefix unused variable with underscore.

The n_components variable is not used in the subsequent logic. Following Python conventions, prefix it with an underscore to indicate it's intentionally unused.

🔎 Proposed fix

-            n_components, labels = connected_components(graph, directed=False)
+            _n_components, labels = connected_components(graph, directed=False)

tests/test_ase.py (1)

109-111: Consider clarifying frozenset construction.

While frozenset({"O", "O"}) is technically correct (frozensets deduplicate automatically), writing frozenset({"O"}) would be clearer and avoid static analysis warnings.

🔎 Proposed fix

         assert frozenset({"H", "O"}) in parsed
-        assert frozenset({"O", "O"}) in parsed
+        assert frozenset({"O"}) in parsed
         assert parsed[frozenset({"H", "O"})] == 1.5
-        assert parsed[frozenset({"O", "O"})] == 2.0
+        assert parsed[frozenset({"O"})] == 2.0

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 943409a and e3b6205.

📒 Files selected for processing (5)

• reacnetgenerator/_detect.py
• reacnetgenerator/commandline.py
• reacnetgenerator/reacnetgen.py
• tests/test_ase.py
• tests/test_ase_cli.py

🧰 Additional context used

🧬 Code graph analysis (3)

tests/test_ase_cli.py (1)

reacnetgenerator/commandline.py (1)

• main_parser (13-153)

tests/test_ase.py (1)

reacnetgenerator/reacnetgen.py (1)

• ReacNetGenerator (67-382)

reacnetgenerator/_detect.py (1)

reacnetgenerator/utils.py (5)

• SharedRNGData (388-423)
• WriteBuffer (44-119)
• listtobytes (217-230)
• run_mp (535-568)
• append (76-85)

🪛 GitHub Actions: Type checker

reacnetgenerator/_detect.py

[error] 218-218: "csr_matrix" is possibly unbound (reportPossiblyUnboundVariable)

🪛 GitHub Check: CodeQL

tests/test_ase.py

[notice] 12-12: Unused import
Import of 'natural_cutoffs' is not used.
Import of 'neighbor_list' is not used.

---

[notice] 19-19: Unused import
Import of 'connected_components' is not used.

🪛 GitHub Check: pyright

reacnetgenerator/_detect.py

[failure] 221-221:
"connected_components" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 218-218:
"csr_matrix" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 441-441:
Cannot access attribute "ase_cutoff_mult" for class "ReacNetGenerator"
  Attribute "ase_cutoff_mult" is unknown (reportAttributeAccessIssue)

---

[failure] 419-419:
"neighbor_list" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 407-407:
Cannot access attribute "ase_cutoff_mult" for class "ReacNetGenerator"
  Attribute "ase_cutoff_mult" is unknown (reportAttributeAccessIssue)

---

[failure] 392-392:
Cannot access attribute "custom_cutoffs" for class "ReacNetGenerator"
  Attribute "custom_cutoffs" is unknown (reportAttributeAccessIssue)

---

[failure] 391-391:
Cannot access attribute "custom_cutoffs" for class "ReacNetGenerator"
  Attribute "custom_cutoffs" is unknown (reportAttributeAccessIssue)

---

[failure] 387-387:
Argument of type "float" cannot be assigned to parameter "mult" of type "int" in function "natural_cutoffs"
  "float" is not assignable to "int" (reportArgumentType)

---

[failure] 387-387:
"natural_cutoffs" is possibly unbound (reportPossiblyUnboundVariable)

🪛 Ruff (0.14.10)

tests/test_ase.py

109-109: Sets should not contain duplicate item "O"

Remove duplicate item

(B033)

---

111-111: Sets should not contain duplicate item "O"

Remove duplicate item

(B033)

---

231-231: Loop control variable component not used within loop body

Rename unused component to _component

(B007)

reacnetgenerator/_detect.py

221-221: Unpacked variable n_components is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

---

383-383: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (14)

reacnetgenerator/reacnetgen.py (2)

171-173: LGTM! Sensible default values for ASE configuration.

The default values align with the CLI argument defaults and provide reasonable starting points for ASE-based bond detection.

---

233-241: LGTM! Well-designed auto-enabling logic.

The automatic activation of ASE mode when custom cutoffs or modified multipliers are provided improves the user experience by reducing configuration overhead.

reacnetgenerator/_detect.py (6)

44-63: LGTM! Proper handling of optional dependencies.

The conditional import pattern with availability flags follows best practices and matches the existing pattern used for OpenBabel.

---

476-478: LGTM! Clean conditional delegation to ASE.

The use of getattr with a default value provides robust fallback behavior when the use_ase attribute is not present.

---

547-547: LGTM! Improved observability for LAMMPS dump parsing.

The additional logging provides useful visibility into the parsing progress and helps with debugging.

Also applies to: 576-576, 583-583

---

448-453: Excellent duplicate bond prevention for PBC.

The duplicate check is crucial for preventing "bond already exists" errors when PBC creates periodic images. This is correctly validated by the test suite.

---

382-384: The method is already properly guarded by callers.

The code at line 477-478 in _detect.py checks if getattr(self.rng, "use_ase", False): before invoking _getbondfromase. The ImportError at lines 382-383 serves as an appropriate defensive check within the method, ensuring it fails gracefully if called incorrectly.

---

387-387: No action needed. ASE's natural_cutoffs function explicitly accepts both int and float for the mult parameter. The code at line 387 correctly passes mult=1.0, which is a valid type.

reacnetgenerator/commandline.py (2)

70-86: LGTM! Well-structured CLI arguments for ASE mode.

The arguments provide clear options for users to control ASE-based bond detection, with sensible defaults that align with the internal configuration.

---

181-183: LGTM! Consistent parameter wiring.

The ASE parameters are correctly passed from CLI arguments to ReacNetGenerator and properly reconstructed in parm2cmd.

Also applies to: 236-241

tests/test_ase_cli.py (1)

1-65: LGTM! Comprehensive CLI argument tests.

The tests thoroughly validate that ASE-related command-line arguments are correctly parsed, have proper defaults, and are documented in help text.

tests/test_ase.py (3)

43-92: LGTM! Thorough tests for ASE mode enabling.

The tests validate default behavior, manual enabling, and auto-enabling logic with custom cutoffs or modified multipliers.

---

134-203: LGTM! Essential tests for ASE bond detection.

The tests validate bond perception and duplicate prevention, which are critical for correct ASE-based bond detection, especially with PBC.

---

237-272: LGTM! Comprehensive integration and warning tests.

The integration test validates end-to-end ASE functionality with custom cutoffs, and the warning test ensures users are notified when ASE mode is auto-enabled.

Also applies to: 274-290

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

tests/test_detect.py (4)

67-73: Remove unused import or use underscore prefix.

The imports natural_cutoffs and neighbor_list are flagged as unused by static analysis. While they appear to be used in the implementation code being tested (reacnetgenerator/_detect.py), they are not directly used in this test file.

🔎 Proposed fix

If these imports are truly unused in this test file, remove them:

 try:
-    from ase.neighborlist import natural_cutoffs, neighbor_list
+    from ase import Atoms
 
     ASE_AVAILABLE = True
 except ImportError:
     ASE_AVAILABLE = False

Or if you're keeping them for future use, prefix with underscore to indicate they're intentionally unused:

-    from ase.neighborlist import natural_cutoffs, neighbor_list
+    from ase.neighborlist import natural_cutoffs as _natural_cutoffs, neighbor_list as _neighbor_list

---

76-77: Remove unused import or use underscore prefix.

The import connected_components is flagged as unused. It's defined but not directly used in this test file.

🔎 Proposed fix

 try:
-    from scipy.sparse.csgraph import connected_components
+    # connected_components is used in the implementation, not in tests
 
     SCIPY_AVAILABLE = True
 except ImportError:
     SCIPY_AVAILABLE = False

---

180-202: Prefix unused variable to suppress linter warning.

The bondlevel variable is unpacked but never used in this test. Consider prefixing it with an underscore to indicate it's intentionally unused.

🔎 Proposed fix

-        bond, bondlevel = detect_instance._getbondfromase(atoms, cell)
+        bond, _bondlevel = detect_instance._getbondfromase(atoms, cell)
 
         # Check that there are no duplicate entries in bond lists
         for i, neighbors in enumerate(bond):

---

236-262: Add assertion on component data to strengthen test.

The test creates component_bytes but doesn't verify its structure or contents. Consider adding assertions to validate the component data format.

🔎 Proposed enhancement

         # The component should contain all atoms
-        component_bytes = result[0]
-        # The first part of the component bytes contains the atom indices
-        # Need to extract the atom list from the bytes format
-        # This is tricky since it's in the internal format, so just check length
-        # The result format is: atom_indices + bond_pairs + bond_levels
         assert len(result) == 1  # Single connected component
+        
+        # Verify the component contains all atoms by checking it's non-empty
+        assert len(result[0]) > 0, "Component data should not be empty"

reacnetgenerator/_detect.py (1)

381-387: Chain exception for better error context.

When re-raising a ValueError after catching one, use raise ... from err to preserve the exception chain and provide better debugging context.

🔎 Proposed fix

         try:
             distance = float(dist_str)
-        except ValueError:
+        except ValueError as err:
             raise ValueError(
                 f"Invalid distance value '{dist_str}' in '{pair}'. Expected a number. "
                 f"Example: 'Al-O:2.5,C-H:1.1'"
-            )
+            ) from err

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b937710 and 884c714.

📒 Files selected for processing (3)

• reacnetgenerator/_detect.py
• tests/test_detect.py
• tests/test_reacnetgen.py

🧰 Additional context used

🧬 Code graph analysis (1)

reacnetgenerator/_detect.py (1)

reacnetgenerator/utils.py (3)

• WriteBuffer (44-119)
• listtobytes (217-230)
• append (76-85)

🪛 GitHub Actions: Type checker

reacnetgenerator/_detect.py

[error] 218-218: pyright: 'csr_matrix' is possibly unbound. (reportPossiblyUnboundVariable)

🪛 GitHub Check: CodeQL

tests/test_detect.py

[notice] 69-69: Unused import
Import of 'natural_cutoffs' is not used.
Import of 'neighbor_list' is not used.

---

[notice] 76-76: Unused import
Import of 'connected_components' is not used.

---

[notice] 257-257: Unused local variable
Variable component_bytes is not used.

🪛 GitHub Check: pyright

reacnetgenerator/_detect.py

[failure] 221-221:
"connected_components" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 218-218:
"csr_matrix" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 468-468:
Cannot access attribute "ase_cutoff_mult" for class "ReacNetGenerator"
  Attribute "ase_cutoff_mult" is unknown (reportAttributeAccessIssue)

---

[failure] 446-446:
"neighbor_list" is possibly unbound (reportPossiblyUnboundVariable)

---

[failure] 434-434:
Cannot access attribute "ase_cutoff_mult" for class "ReacNetGenerator"
  Attribute "ase_cutoff_mult" is unknown (reportAttributeAccessIssue)

---

[failure] 419-419:
Cannot access attribute "custom_cutoffs" for class "ReacNetGenerator"
  Attribute "custom_cutoffs" is unknown (reportAttributeAccessIssue)

---

[failure] 418-418:
Cannot access attribute "custom_cutoffs" for class "ReacNetGenerator"
  Attribute "custom_cutoffs" is unknown (reportAttributeAccessIssue)

---

[failure] 414-414:
Argument of type "float" cannot be assigned to parameter "mult" of type "int" in function "natural_cutoffs"
  "float" is not assignable to "int" (reportArgumentType)

---

[failure] 414-414:
"natural_cutoffs" is possibly unbound (reportPossiblyUnboundVariable)

🪛 Ruff (0.14.10)

tests/test_detect.py

214-214: Unpacked variable bondlevel is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

---

257-257: Local variable component_bytes is assigned to but never used

Remove assignment to unused variable component_bytes

(F841)

reacnetgenerator/_detect.py

362-365: Avoid specifying long messages outside the exception class

(TRY003)

---

372-375: Avoid specifying long messages outside the exception class

(TRY003)

---

384-387: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

384-387: Avoid specifying long messages outside the exception class

(TRY003)

---

410-410: 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). (4)

• GitHub Check: Build wheels for cp38-win_amd64
• GitHub Check: Build wheels for cp310-manylinux_aarch64
• GitHub Check: Test with tox
• GitHub Check: Benchmark with codspeed

🔇 Additional comments (9)

tests/test_reacnetgen.py (1)

170-232: LGTM! Well-structured tests for ASE auto-enable logic.

The test class validates the expected auto-enable behavior when ASE-related parameters are provided despite use_ase=False. The three test cases cover the key scenarios:

1. Custom cutoffs trigger auto-enable
2. Modified multiplier triggers auto-enable
3. Default values do not trigger auto-enable

tests/test_detect.py (2)

85-138: LGTM! Comprehensive validation of custom cutoff parsing.

The test class thoroughly validates the _parse_custom_cutoffs method with:

• Valid simple and multiple pair formats
• Whitespace handling
• Error cases (missing colon, missing dash, invalid distance)
• Helpful error messages

---

265-300: LGTM! Auto-enable tests provide good coverage.

These tests validate the auto-enable behavior for ASE mode, covering both positive cases (custom cutoffs and modified multiplier) and the negative case (defaults).

reacnetgenerator/_detect.py (6)

48-63: LGTM! Conditional imports properly structured.

The ASE and SciPy imports are correctly guarded with try-except blocks and availability flags. The static analysis warnings about "possibly unbound" are false positives—all imported symbols are used only within their respective availability checks.

---

203-267: LGTM! Well-implemented SciPy-based connected components with proper fallback.

The implementation elegantly uses SciPy's sparse graph algorithms when available and falls back to the existing DPS implementation otherwise. The code correctly:

• Builds a CSR adjacency matrix from bond lists
• Computes connected components
• Groups atoms by component
• Extracts bonds within each component (avoiding duplicates by checking atom_idx < neighbor_idx)
• Serializes components in the expected format

The static analysis warning about csr_matrix being "possibly unbound" (line 218) is a false positive—it's imported in the try block and only used within the SCIPY_AVAILABLE guard.

---

338-390: LGTM! Robust parsing with clear error messages.

The _parse_custom_cutoffs method properly validates user input and provides helpful error messages for malformed strings. The implementation correctly:

• Handles empty/None input
• Strips whitespace to allow flexible formatting
• Validates presence of colon and dash separators
• Validates numeric distance values
• Returns a frozenset-keyed dictionary for order-independent pair lookups

This addresses the past review comment requesting error handling for malformed inputs.

---

392-482: LGTM! Comprehensive ASE-based bond detection with custom cutoffs.

The _getbondfromase method implements a robust bond detection algorithm:

• Computes natural covalent radii as a baseline
• Parses and applies custom per-pair cutoffs
• Calculates an appropriate global search cutoff (max of base + custom with buffer)
• Uses ASE neighbor lists with PBC support
• Applies pair-specific thresholds (custom if available, else scaled radii)
• Deduplicates bonds (line 475 check prevents duplicate bonds from PBC images)

The implementation correctly handles edge cases like empty atom lists and integrates well with the existing codebase.

---

503-505: LGTM! Clean delegation to ASE-based detection.

The conditional check properly routes to ASE-based bond detection when enabled, preserving the original Open Babel path otherwise.

---

574-574: LGTM! Helpful logging for LAMMPS dump parsing.

The logging statements improve observability during file parsing, making it easier to diagnose issues with trajectory files.

Also applies to: 603-603, 610-610

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

tests/test_detect.py (1)

249-254: Remove unused variable assignment.

The variable component_bytes is assigned but never used. The subsequent assertion only checks len(result), not the variable itself.

🔎 Proposed fix

-        # The component should contain all atoms
-        component_bytes = result[0]
-        # The first part of the component bytes contains the atom indices
-        # Need to extract the atom list from the bytes format
-        # This is tricky since it's in the internal format, so just check length
-        # The result format is: atom_indices + bond_pairs + bond_levels
         assert len(result) == 1  # Single connected component

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 884c714 and 23edf0e.

📒 Files selected for processing (1)

• tests/test_detect.py

🧰 Additional context used

🪛 GitHub Check: CodeQL

tests/test_detect.py

[warning] 76-76: Unreachable code
This statement is unreachable.

---

[notice] 257-257: Unused local variable
Variable component_bytes is not used.

🪛 Ruff (0.14.10)

tests/test_detect.py

249-249: Local variable component_bytes is assigned to but never used

Remove assignment to unused variable component_bytes

(F841)

⏰ 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). (4)

• GitHub Check: Build wheels for cp38-win_amd64
• GitHub Check: Build wheels for cp310-manylinux_aarch64
• GitHub Check: Benchmark with codspeed
• GitHub Check: Test with tox

🔇 Additional comments (4)

tests/test_detect.py (4)

25-28: LGTM! Typo fix.

The fixture variable rename from start_direcroty to start_directory corrects the typo.

---

82-135: LGTM! Comprehensive test coverage.

The test cases thoroughly cover valid input formats, edge cases with whitespace, and error scenarios with descriptive assertion messages.

---

137-211: LGTM! Well-structured ASE bond detection tests.

The test cases effectively validate bond detection logic for water molecules, duplicate prevention with periodic boundary conditions, and custom cutoff override behavior.

---

257-292: LGTM! Auto-enable logic tests are well-designed.

The tests effectively verify that ASE mode is automatically enabled when relevant parameters (custom cutoffs or modified multiplier) are provided, even when use_ase is explicitly set to False.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

tests/test_detect.py (1)

249-257: Remove unused variable and duplicate assertion.

The variable component_bytes is assigned but never used, and the assertion at line 257 duplicates line 249. Either complete the verification logic or clean up the dead code.

🔎 Proposed fix

         # Should have 1 component containing all atoms
         assert len(result) == 1
-
-        # The component should contain all atoms
-        component_bytes = result[0]
-        # The first part of the component bytes contains the atom indices
-        # Need to extract the atom list from the bytes format
-        # This is tricky since it's in the internal format, so just check length
-        # The result format is: atom_indices + bond_pairs + bond_levels
-        assert len(result) == 1  # Single connected component

🧹 Nitpick comments (1)

tests/test_detect.py (1)

74-80: Remove the debug print statement.

The print(scipy.__version__) statement pollutes test output and serves no functional purpose. The SciPy availability check works correctly without it.

🔎 Proposed fix

 try:
     import scipy
 
-    print(scipy.__version__)
     SCIPY_AVAILABLE = True
 except ImportError:
     SCIPY_AVAILABLE = False

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23edf0e and ff0e869.

📒 Files selected for processing (1)

• tests/test_detect.py

🧰 Additional context used

🪛 GitHub Check: CodeQL

tests/test_detect.py

[notice] 257-257: Unused local variable
Variable component_bytes is not used.

🪛 Ruff (0.14.10)

tests/test_detect.py

252-252: Local variable component_bytes is assigned to but never used

Remove assignment to unused variable component_bytes

(F841)

🔇 Additional comments (4)

tests/test_detect.py (4)

25-28: LGTM!

The typo fix from start_direcroty to start_directory improves code clarity and consistency.

---

85-138: LGTM!

Comprehensive test coverage for the _parse_custom_cutoffs method with clear scenarios covering valid formats, edge cases with spaces, and various error conditions with helpful message assertions.

---

140-214: LGTM!

Well-structured tests for ASE-based bond detection covering water molecule bonds, PBC duplicate prevention, and custom cutoff overrides. The skipif decorator properly guards against missing ASE.

---

260-295: LGTM!

Good test coverage for the auto-enable logic, verifying that ASE mode is automatically enabled when custom_cutoffs or ase_cutoff_mult parameters are provided, even if use_ase=False is explicitly set.

[njzjz]

Please fix the CIs