Conversation
for more information, see https://pre-commit.ci
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
==========================================
- Coverage 82.85% 82.69% -0.17%
==========================================
Files 25 25
Lines 1820 1832 +12
==========================================
+ Hits 1508 1515 +7
- Misses 312 317 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@dgehringer I finally got around updating |
There was a problem hiding this comment.
Pull request overview
Updates structuretoolkit’s SQS generation integration and tests to work with a newer sqsgenerator release.
Changes:
- Bumped
sqsgeneratorin CI conda environments to newer pinned versions. - Refactored
sqs_structuresto use the newersqsgenerator.optimize/to_aseAPI and new settings schema. - Adjusted SQS unit tests to match new statistics/SRO outputs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/structuretoolkit/build/sqs.py |
Reworks SQS generation to call the new sqsgenerator API and changes how structures/SRO/timings are produced. |
tests/test_sqs.py |
Updates expectations and call signature for sqs_structures under the new sqsgenerator behavior. |
.ci_support/environment.yml |
Pins sqsgenerator to 0.5.4 in the main CI environment. |
.ci_support/environment-old.yml |
Pins sqsgenerator to 0.4 in the “old” CI environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| structures = [] | ||
| finished = False | ||
| for r_lst in result: | ||
| if not finished: | ||
| for s in r_lst[1]: | ||
| structures.append(to_ase(s.structure())) | ||
| if len(structures) == output_structures: | ||
| finished = True | ||
| break |
There was a problem hiding this comment.
After finished becomes true, the outer for r_lst in result: loop continues iterating even though no more structures are collected. This can do unnecessary work (and potentially consume a large iterator); break out of the outer loop once the requested output_structures have been gathered.
| break | ||
|
|
||
| sro_breakdown = [s.sro() for s in result[0][1]] | ||
| cycle_time = list(result.statistics.timings.values())[0] |
There was a problem hiding this comment.
cycle_time = list(result.statistics.timings.values())[0] is fragile: it will raise if timings is empty, and it picks an arbitrary timing entry based on dict insertion order rather than a well-defined metric (the previous implementation averaged timings). Consider selecting a specific timing key or aggregating deterministically (e.g., average or sum) so the returned cycle_time is stable and meaningful.
| cycle_time = list(result.statistics.timings.values())[0] | |
| timings = list(result.statistics.timings.values()) | |
| cycle_time = float(np.mean(timings)) if timings else 0.0 |
| self.assertEqual(num_iterations, 1000000) | ||
| self.assertTrue(cycle_time < 100000000000) |
There was a problem hiding this comment.
self.assertTrue(cycle_time < 100000000000) effectively stops testing anything about performance/regressions (almost any value will pass). If runtime is too variable to bound tightly, consider asserting basic invariants (e.g., cycle_time >= 0 and finite) or mocking/stubbing timing rather than using an extremely large upper bound.
| def sqs_structures( | ||
| structure: Atoms, | ||
| mole_fractions: dict[str, float | int], | ||
| weights: dict[int, float] | None = None, | ||
| objective: float | np.ndarray = 0.0, | ||
| iterations: float | int = 1e6, | ||
| output_structures: int = 10, | ||
| mode: str = "random", | ||
| num_threads: int | None = None, | ||
| prefactors: float | np.ndarray | None = None, | ||
| pair_weights: np.ndarray | None = None, | ||
| rtol: float | None = None, | ||
| atol: float | None = None, | ||
| which: Iterable[int] | None = None, | ||
| shell_distances: Iterable[int] | None = None, | ||
| minimal: bool | None = True, | ||
| similar: bool | None = True, | ||
| return_statistics: bool | None = False, | ||
| ) -> Atoms | tuple[Atoms, dict[str, list], int, float]: |
There was a problem hiding this comment.
sqs_structures is part of the public API (re-exported from structuretoolkit.build and top-level structuretoolkit). Removing parameters like prefactors, pair_weights, which, etc. is a breaking change for downstream callers; consider keeping them as deprecated keyword args (ignored or mapped to the new sqsgenerator API) with a warning, or bumping the major version and documenting the break explicitly.
| @@ -188,54 +182,54 @@ | |||
| output_structures (int): The number of output structures. | |||
| mode (str): The mode for selecting configurations. | |||
| num_threads (Optional[int]): The number of threads to use. | |||
| prefactors (Optional[Union[float, np.ndarray]]): The prefactors for each shell. | |||
| pair_weights (Optional[np.ndarray]): The pair weights. | |||
| rtol (Optional[float]): The relative tolerance. | |||
| atol (Optional[float]): The absolute tolerance. | |||
| which (Optional[Iterable[int]]): The indices of the shells to optimize. | |||
| shell_distances (Optional[Iterable[int]]): The distances for each shell. | |||
| minimal (Optional[bool]): Whether to minimize the objective function. | |||
| similar (Optional[bool]): Whether to generate similar structures. | |||
| return_statistics (Optional[bool]): Whether to return additional statistics. | |||
|
|
|||
| Returns: | |||
| Union[Atoms, Tuple[Atoms, Dict[str, list], int, float]]: The generated structures or a tuple containing the structures, short-range order parameters breakdown, number of iterations, and average cycle time. | |||
|
|
|||
There was a problem hiding this comment.
The return type annotation and docstring say this returns an Atoms (or tuple with Atoms), but the implementation returns a list[Atoms] (and sro_breakdown is no longer a dict[str, list]). Please update the type hints and docstring to match the actual return types so callers and static checkers don’t get misled.
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/structuretoolkit/build/sqs.py`:
- Around line 219-231: The sro_breakdown is derived from result[0][1] regardless
of which SqsResult entries were actually appended to structures; change the loop
that builds structures (the block using structures, finished, result,
output_structures, and to_ase) to also collect the corresponding SqsResult
objects (e.g., selected_results) when you append a structure, and then compute
sro_breakdown from that same selection (sro_breakdown = [s.sro() for s in
selected_results]) instead of using result[0][1]; this ensures SRO data aligns
with the returned structures even when items are taken from multiple buckets or
truncated.
In `@tests/test_sqs.py`:
- Around line 63-66: Replace the brittle hard-coded stats assertions: instead of
asserting sro.shape == (5,2,2) and cycle_time < 100000000000, assert invariants
that don't depend on current shell count: check len(sro_breakdown) ==
len(structures_lst), for each sro assert sro.shape[1:] == (2, 2), and assert
cycle_time is a finite non-negative value (e.g., use math.isfinite(cycle_time)
and cycle_time >= 0); keep the num_iterations assertion if needed
(num_iterations == 1000000) or verify it against the test setup variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30dd9f07-65d2-418e-a831-fcbd46b90908
📒 Files selected for processing (4)
.ci_support/environment-old.yml.ci_support/environment.ymlsrc/structuretoolkit/build/sqs.pytests/test_sqs.py
| for sro in sro_breakdown: | ||
| self.assertEqual(list(sro.keys()), ["Cu-Cu", "Cu-Au", "Au-Au"]) | ||
| self.assertEqual(num_iterations, 1000000.0) | ||
| self.assertTrue(cycle_time < 10) | ||
| self.assertEqual((5, 2, 2), sro.shape) | ||
| self.assertEqual(num_iterations, 1000000) | ||
| self.assertTrue(cycle_time < 100000000000) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the test file
find . -name "test_sqs.py" -type fRepository: pyiron/structuretoolkit
Length of output: 86
🏁 Script executed:
# Read the test file to understand the context
cat -n ./tests/test_sqs.py | head -100Repository: pyiron/structuretoolkit
Length of output: 2758
🏁 Script executed:
# Search for the sqs_structures function implementation
rg -n "def sqs_structures" --type pyRepository: pyiron/structuretoolkit
Length of output: 124
🏁 Script executed:
# Read the sqs_structures function
sed -n '160,300p' ./src/structuretoolkit/build/sqs.pyRepository: pyiron/structuretoolkit
Length of output: 3144
🏁 Script executed:
# Check if there's documentation or comments about sro shape
rg -n "sro\(\)" --type py -B 3 -A 3Repository: pyiron/structuretoolkit
Length of output: 555
🏁 Script executed:
# Look at the structure more carefully - check what result[0][1] vs all r_lst[1] contains
sed -n '215,235p' ./src/structuretoolkit/build/sqs.pyRepository: pyiron/structuretoolkit
Length of output: 817
🏁 Script executed:
# Check the full context around how structures are populated vs sro_breakdown
sed -n '217,235p' ./src/structuretoolkit/build/sqs.pyRepository: pyiron/structuretoolkit
Length of output: 668
🏁 Script executed:
# Look at how the structures loop works and what result[0][1] contains
# Let me examine if there's a mismatch between returned structures and sro_breakdown
sed -n '217,235p' ./src/structuretoolkit/build/sqs.py | cat -nRepository: pyiron/structuretoolkit
Length of output: 801
🏁 Script executed:
# Let me understand what structures_lst returns and what sro_breakdown contains
# by looking at the return statement more carefully
sed -n '160,240p' ./src/structuretoolkit/build/sqs.py | grep -A 5 "sro_breakdown\|structures ="Repository: pyiron/structuretoolkit
Length of output: 536
🏁 Script executed:
# Let me understand the potential mismatch: structures stops at output_structures but sro_breakdown uses all of result[0][1]
# Let me check the test expectations more carefully
sed -n '41,66p' ./tests/test_sqs.pyRepository: pyiron/structuretoolkit
Length of output: 1139
🏁 Script executed:
# Let me check if there's a potential mismatch between structures and sro_breakdown
# by understanding the structure better. The key issue: does structures get limited to 10
# but sro_breakdown uses all of result[0][1]?
# First, let's trace through: output_structures=10 means the loop breaks after 10 structures
# But sro_breakdown = [s.sro() for s in result[0][1]] uses ALL of result[0]'s structures
# This could be a mismatch if result[0] has more than 10 structures, or if structures
# come from multiple objectives (result[0], result[1], etc.)
# Let me search for sqsgenerator documentation or usage hints
rg -n "result\[0\]" --type py -B 2 -A 2Repository: pyiron/structuretoolkit
Length of output: 442
🏁 Script executed:
# Let me verify if there's a potential mismatch by looking at the loop logic more carefully
# The key observation: structures breaks after 10, but sro_breakdown uses result[0][1] completely
sed -n '217,235p' ./src/structuretoolkit/build/sqs.pyRepository: pyiron/structuretoolkit
Length of output: 668
🏁 Script executed:
# Let me check the actual structure of what sqsgenerator returns
# by looking for any related imports or type hints
rg -n "from sqsgenerator import\|import sqsgenerator" --type py -A 5Repository: pyiron/structuretoolkit
Length of output: 49
🏁 Script executed:
# Let me understand the potential mismatch between structures_lst and sro_breakdown
# The key question: does result[0][1] always have exactly output_structures items?
# Or can it have more/fewer?
# Let me trace the logic:
# - structures stops after output_structures (10) items are added
# - sro_breakdown uses ALL of result[0][1]
# These should match but might not
# Let me search for any related issues or comments about this
rg -n "len(structures_lst)" --type py -B 2 -A 2Repository: pyiron/structuretoolkit
Length of output: 49
🏁 Script executed:
# Let me verify the critical mismatch: the structures loop stops at output_structures,
# but sro_breakdown uses result[0][1] in its entirety. Are they the same length?
# The key issue:
# - structures breaks when len(structures) == output_structures (10)
# - sro_breakdown = [s.sro() for s in result[0][1]] uses ALL of result[0][1]
#
# These might not match! Let me verify by checking the test assertions
cat -n ./tests/test_sqs.py | sed -n '41,70p'Repository: pyiron/structuretoolkit
Length of output: 1321
🏁 Script executed:
# Let me trace through the critical mismatch more carefully.
# The structures loop breaks after 10 items, but sro_breakdown uses result[0][1] entirely.
# Are they guaranteed to be the same length?
# Let me check if there's any code that ensures they match
sed -n '217,235p' ./src/structuretoolkit/build/sqs.py | python3 << 'EOF'
import sys
code = sys.stdin.read()
print("=== Code Analysis ===")
print("Loop: breaks after len(structures) == output_structures (10)")
print("sro_breakdown: [s.sro() for s in result[0][1]]")
print("\nPotential Mismatch:")
print("- structures can have UP TO output_structures items from ANY objectives")
print("- sro_breakdown uses ALL items from result[0][1] only")
print("\nIf structures come from multiple objectives or result[0] has > 10:")
print(" len(structures_lst) != len(sro_breakdown)")
print("\nIf result[0] has exactly 10 and all structures come from result[0]:")
print(" They match (coincidentally)")
EOFRepository: pyiron/structuretoolkit
Length of output: 554
These stats assertions are brittle and weak.
(5, 2, 2) hard-codes the current shell count even though shell selection is auto-derived when no shell settings are supplied, and cycle_time < 100000000000 is too loose to catch a regression in the new timing extraction. Prefer invariant checks here, e.g. len(sro_breakdown) == len(structures_lst), sro.shape[1:] == (2, 2), and a finite non-negative cycle_time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_sqs.py` around lines 63 - 66, Replace the brittle hard-coded stats
assertions: instead of asserting sro.shape == (5,2,2) and cycle_time <
100000000000, assert invariants that don't depend on current shell count: check
len(sro_breakdown) == len(structures_lst), for each sro assert sro.shape[1:] ==
(2, 2), and assert cycle_time is a finite non-negative value (e.g., use
math.isfinite(cycle_time) and cycle_time >= 0); keep the num_iterations
assertion if needed (num_iterations == 1000000) or verify it against the test
setup variable.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hi @jan-janssen! Thanks a lot for tackling this. Yes for a basic interface that looks good to me |
Summary by CodeRabbit
Chores
Breaking Changes
Tests