fix(doc): doc readme about tests, reproducing, demo and manual document - #162
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new PyMOL measurement-parsing utility, updates docs and README content, adjusts pre-commit Black hook arguments, updates test data references and a test import ordering, replaces a license script with a top-level NotImplementedError, and a small change in a CGO utility's printed docstring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MeasurementModule as Measurement
participant PyMOL
participant GromacsOutput as Gromacs
User->>Measurement: request read_measurement(start)
Measurement->>PyMOL: query session names / measurement entries
PyMOL-->>Measurement: returns measurement pylist(s)
Measurement->>Measurement: parse DistSet / MeasureInfo, build scene AtomDescriptor list
Measurement->>PyMOL: request object atom coords / unique ids (when needed)
PyMOL-->>Measurement: atom attributes (coords, ids, resn, resi, chain)
Measurement->>Measurement: resolve atoms (unique id OR nearest-by-coord)
Measurement->>Gromacs: emit index/group strings (Gromacs-like)
Measurement-->>User: return Measurement + printed mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
  DeepSource reviewed changes in the commit range For detailed review results, please see the PR on DeepSource ↗ PR Report Card
Code Review Summary
How are these analyzer statuses calculated?Administrators can configure which issue categories are reported and cause analysis to be marked as failed when detected. This helps prevent bad and insecure code from being introduced in the codebase. If you're an administrator, you can modify this in the repository's settings. |
❌ 13 blocking issues (31 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| 2. call the extended command: `read_measurement [start,[debug]]` | ||
| """ | ||
|
|
||
| # TODO: |
| """ | ||
|
|
||
|
|
||
| import math |
|
|
||
|
|
||
| import math | ||
| from collections.abc import Iterable, Sequence |
|
|
||
| import math | ||
| from collections.abc import Iterable, Sequence | ||
| from dataclasses import dataclass, field |
| import math | ||
| from collections.abc import Iterable, Sequence | ||
| from dataclasses import dataclass, field | ||
| from typing import Any, Dict, List, Optional, Tuple |
There was a problem hiding this comment.
Found 4 issues:
1. typing.List imported but unused [ruff:F401]
2. typing.Tuple imported but unused [ruff:F401]
3. Module level import not at top of file [ruff:E402]
4. typing.Dict imported but unused [ruff:F401]
| from typing import Any, Dict, List, Optional, Tuple | |
| from typing import Any, Optional |
| except Exception: | ||
| try: | ||
| model = cmd_module.get_model(obj) | ||
| except Exception: |
| ) | ||
| # Optionally: ignore matches that are far away (very large distance) | ||
| if best is not None: | ||
| return best |
| # | ||
|
|
||
|
|
||
| def read_measurement(start: str | int, debug: int = 0) -> Measurement: |
| print(f'labels=({" ".join(names)})') | ||
| print(f'grp_as=({" ".join(atom_a)})') | ||
| print(f'grp_bs=({" ".join(atom_b)})') | ||
|
|
| return hits | ||
|
|
||
|
|
||
| cmd.extend("read_measurement", read_measurement) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf1e151fbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import pytest | ||
|
|
||
| pytest.importorskip("PyQt5") | ||
| from PyQt5 import QtCore, QtWidgets |
There was a problem hiding this comment.
This file now imports PyQt5 before calling pytest.importorskip("PyQt5"), so environments that intentionally lack PyQt5 will raise ModuleNotFoundError during module import and fail collection instead of skipping the test module. That regresses optional-dependency behavior and can break CI/test runs that previously passed by skipping GUI tests.
Useful? React with 👍 / 👎.
| # re-organize the strings | ||
| names = [f"'{x}'" for x in pairs.keys()] | ||
| atom_a = [f"'{x[0]}'" for x in pairs.values()] | ||
| atom_b = [f"'{x[1]}'" for x in pairs.values()] |
There was a problem hiding this comment.
read_measurement assumes every parsed measurement has at least two resolved residues, but Measurement._collect_unique_ids() deduplicates IDs, so a self-distance (or any measurement resolving to one atom) can produce a one-element pair; indexing x[1] then raises IndexError and aborts the command. Add a length check (or preserve duplicate IDs) before building atom_b.
Useful? React with 👍 / 👎.
[ci skip]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 279: Update the README heading that currently reads "## Formating" to the
correct spelling "## Formatting" by editing the header string in the README
(look for the header "## Formating") so the typo is fixed.
- Line 228: Typo in the README sentence: replace the misspelled word "bellow"
with "below" in the sentence "To reproduce our data, please refer to our
manuscript as well as the input data section bellow." to correct the spelling
and update the README accordingly.
In `@src/REvoDesign/tools/measure_utils.py`:
- Around line 671-674: The ValueError raised when "not hits" references the
empty hits list and yields "[]", so update the ValueError in the block that
checks hits to include the session's available measurement names (e.g., use
[m.name for m in session.measurements] or the appropriate session attribute)
instead of [m.name for m in hits]; change the f-string in the ValueError to show
the requested measurement identifier and the list of available names from
session (fall back to a safe repr of session if session.measurements isn't
present) so diagnostics are useful.
- Around line 642-714: Change the return type of read_measurement to return a
sequence (e.g., list[Measurement]) instead of Measurement (update the function
signature), and make the post-processing that builds atom_a and atom_b
resilient: when iterating pairs.values() (from Measurement.from_session_names ->
hits) guard accesses to x[0] and x[1] (e.g., conditional expressions or skips)
so you don't IndexError on pairs with fewer than two atoms; keep returning hits
as before. Ensure you update the signature for read_measurement and the logic
that builds atom_a/atom_b from pairs.values().
In `@tests/test_thread_ui_bridge.py`:
- Around line 1-6: Move the pytest.importorskip("PyQt5") guard to before any
direct imports from PyQt5 so the skip can take effect; specifically, place
pytest.importorskip("PyQt5") at the top of the file before the line "from PyQt5
import QtCore, QtWidgets", then import PyQt5 symbols and
REvoDesign.tools.package_manager after that guard. This ensures
pytest.importorskip is evaluated before the "from PyQt5 import QtCore,
QtWidgets" import that would raise ImportError.
🧹 Nitpick comments (2)
src/REvoDesign/tools/measure_utils.py (2)
73-73: Unused imports:Dict,List,Tuplefromtyping.The code uses modern
dict[...],list[...],tuple[...]syntax throughout. These legacy typing aliases are never referenced.Fix
-from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Optional
481-619: Remove dead code:_build_uniqueid_to_atom_mapand_resolve_by_coordsare unused.Neither method is called anywhere in this codebase. They duplicate logic already present in
_build_scene_atom_list,_nearest_atom_by_coord, andatoms(). The issues:
_build_uniqueid_to_atom_mapinconsistently falls back toid/serialattributes, contradicting_build_scene_atom_list(line 274-280) which explicitly avoids such fallbacks with the comment "do NOT fall back to 'id' or 'serial'"_resolve_by_coordshas an unused parameteruid(ARG002)These methods add maintenance burden without providing value. Remove them.
| def read_measurement(start: str | int, debug: int = 0) -> Measurement: | ||
| """ | ||
| This function reads the measurement from the PyMOL session and prints atoms as gromacs index strings. | ||
|
|
||
| Parameters | ||
| start : str or int | ||
| The starting atom index. | ||
| debug : bool int, optional | ||
| If non-zero, prints debug information. The default is 0. | ||
|
|
||
|
|
||
| ``` | ||
| 9 & r 111 ; select the non-glycine residue sidechain group at 111 | ||
| name 2 r111 ; rename it from 2 to r111 | ||
| 8 & r 514 ; select the glycine residue backbone group at 514 | ||
| name 3 r514 ; rename it from 3 to r514 | ||
| ``` | ||
| """ | ||
| DEBUG = bool(int(debug)) | ||
|
|
||
| start = int(start) | ||
| from pymol import cmd | ||
|
|
||
| atoms: dict[int, str] = {} | ||
| pairs: dict = {} | ||
|
|
||
| session = cmd.get_session() | ||
| hits = Measurement.from_session_names(session["names"]) | ||
|
|
||
| if not hits: | ||
| raise ValueError( | ||
| f"measurement not found in session {[m.name for m in hits]}", | ||
| ) | ||
|
|
||
| for hit in hits: | ||
| if DEBUG: | ||
| print("-=" * 30) | ||
| print(f"[DEBUG] {hit.summarize(cmd)}") | ||
| pair = [] | ||
| for a in hit.atoms(cmd): | ||
| # | ||
| pair.append(f"r{a.resi}") | ||
|
|
||
| if f"r{a.resi}" in atoms.values(): | ||
| if DEBUG: | ||
| print(f"[DEBUG] skiping {a.resi} to avoid duplicates") | ||
| continue | ||
|
|
||
| start += 1 | ||
|
|
||
| print(f'{"8" if a.resn == "GLY" else "9"} & r {a.resi}') | ||
| print(f"name {start} r{a.resi}") | ||
|
|
||
| atoms[start] = f"r{a.resi}" | ||
|
|
||
| pairs[hit.name] = pair | ||
| if DEBUG: | ||
| print(f"[DEBUG] {hit.name} {pair}") | ||
| print("-=" * 30) | ||
|
|
||
| if DEBUG: | ||
| print(pairs) | ||
| print("-=" * 30) | ||
| # re-organize the strings | ||
| names = [f"'{x}'" for x in pairs.keys()] | ||
| atom_a = [f"'{x[0]}'" for x in pairs.values()] | ||
| atom_b = [f"'{x[1]}'" for x in pairs.values()] | ||
|
|
||
| print(f'labels=({" ".join(names)})') | ||
| print(f'grp_as=({" ".join(atom_a)})') | ||
| print(f'grp_bs=({" ".join(atom_b)})') | ||
|
|
||
| return hits |
There was a problem hiding this comment.
Return type annotation is wrong — function returns list[Measurement], not Measurement.
Line 642 declares -> Measurement but Line 714 returns hits, which is list["Measurement"] (from Measurement.from_session_names). Every caller will get the wrong type hint.
Additionally, Lines 707-708 assume every measurement pair has at least 2 atoms (x[0], x[1]). If a measurement resolves to fewer atoms, this will raise an IndexError.
Proposed fix for return type and safety check
-def read_measurement(start: str | int, debug: int = 0) -> Measurement:
+def read_measurement(start: str | int, debug: int = 0) -> list[Measurement]:And guard the pair indexing:
names = [f"'{x}'" for x in pairs.keys()]
- atom_a = [f"'{x[0]}'" for x in pairs.values()]
- atom_b = [f"'{x[1]}'" for x in pairs.values()]
+ atom_a = [f"'{x[0]}'" for x in pairs.values() if len(x) > 0]
+ atom_b = [f"'{x[1]}'" for x in pairs.values() if len(x) > 1]🧰 Tools
🪛 Ruff (0.15.0)
[warning] 672-674: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/REvoDesign/tools/measure_utils.py` around lines 642 - 714, Change the
return type of read_measurement to return a sequence (e.g., list[Measurement])
instead of Measurement (update the function signature), and make the
post-processing that builds atom_a and atom_b resilient: when iterating
pairs.values() (from Measurement.from_session_names -> hits) guard accesses to
x[0] and x[1] (e.g., conditional expressions or skips) so you don't IndexError
on pairs with fewer than two atoms; keep returning hits as before. Ensure you
update the signature for read_measurement and the logic that builds
atom_a/atom_b from pairs.values().
| if not hits: | ||
| raise ValueError( | ||
| f"measurement not found in session {[m.name for m in hits]}", | ||
| ) |
There was a problem hiding this comment.
Error message references the empty hits list — unhelpful diagnostic.
When not hits is true, hits is [], so the message f"measurement not found in session {[m.name for m in hits]}" always produces "measurement not found in session []". Consider listing the session's available names instead.
Suggested fix
if not hits:
raise ValueError(
- f"measurement not found in session {[m.name for m in hits]}",
+ "No measurement objects found in the current PyMOL session.",
)🧰 Tools
🪛 Ruff (0.15.0)
[warning] 672-674: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/REvoDesign/tools/measure_utils.py` around lines 671 - 674, The ValueError
raised when "not hits" references the empty hits list and yields "[]", so update
the ValueError in the block that checks hits to include the session's available
measurement names (e.g., use [m.name for m in session.measurements] or the
appropriate session attribute) instead of [m.name for m in hits]; change the
f-string in the ValueError to show the requested measurement identifier and the
list of available names from session (fall back to a safe repr of session if
session.measurements isn't present) so diagnostics are useful.
| import pytest | ||
|
|
||
| pytest.importorskip("PyQt5") | ||
| from PyQt5 import QtCore, QtWidgets | ||
|
|
||
| from REvoDesign.tools import package_manager | ||
|
|
||
| pytest.importorskip("PyQt5") |
There was a problem hiding this comment.
pytest.importorskip is placed after the unconditional PyQt5 import — skip never fires.
from PyQt5 import QtCore, QtWidgets on Line 2 will raise ImportError before pytest.importorskip("PyQt5") on Line 6 has a chance to skip the module. The guard must come first.
Proposed fix
import pytest
+
+pytest.importorskip("PyQt5")
+
from PyQt5 import QtCore, QtWidgets
from REvoDesign.tools import package_manager
-pytest.importorskip("PyQt5")
-📝 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 pytest | |
| pytest.importorskip("PyQt5") | |
| from PyQt5 import QtCore, QtWidgets | |
| from REvoDesign.tools import package_manager | |
| pytest.importorskip("PyQt5") | |
| import pytest | |
| pytest.importorskip("PyQt5") | |
| from PyQt5 import QtCore, QtWidgets | |
| from REvoDesign.tools import package_manager |
🤖 Prompt for AI Agents
In `@tests/test_thread_ui_bridge.py` around lines 1 - 6, Move the
pytest.importorskip("PyQt5") guard to before any direct imports from PyQt5 so
the skip can take effect; specifically, place pytest.importorskip("PyQt5") at
the top of the file before the line "from PyQt5 import QtCore, QtWidgets", then
import PyQt5 symbols and REvoDesign.tools.package_manager after that guard. This
ensures pytest.importorskip is evaluated before the "from PyQt5 import QtCore,
QtWidgets" import that would raise ImportError.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores