Conversation
…pdate dependencies - Added `is_symmetric`, `asupos`, and `positioned_atoms` properties to `Body` and `SymBody` - Improved validation in `BodyContacts` to reject redundant comparisons - Included `tokens1` and `tokens2` metadata in `ContactMatrixStack`; enforced nonempty contact matrix - Exposed `symbody()` and `symbodies()` accessors on `Components` - Refined `func_params` docstring and `picklocals` frame logic for Python <3.12 - Updated `@iterize_on_first_param` decorator to pass `wrap_ctx` to `info()` - Bumped version to 0.3.9; upgraded `httpx`, `hydra-core`, `omegaconf` and pinned `torch==2.6.0` - Removed `pytest-benchmark` and `py-cpuinfo` from dev dependencies - Added documentation link to `README.md`
…lity from `ipd.dev.cuda` to `ipd.cuda` BREAKING CHANGE: The `ipd._prelude.chrono` module and the `global_chrono` mechanism have been removed. All CUDA-related imports formerly accessed via `ipd.dev.cuda` are now found under `ipd.cuda`. This affects lazy imports, voxel utilities, RMS scanning, and CUDA extension building. Any direct usage of `chrono`, `@chrono`, or `checkpoint()` must be updated or removed. Additionally: - Deleted `chrono.py` and its associated tests - Updated `lazyimport` to support fallback tuple syntax - Adjusted tests and CUDA modules to reflect new package structure - Deprecated `fast` pytest marker in favor of clearer alternatives
…ine component merging - Enable generic `SymAdapt` dispatch for all dataclasses by: - Introducing a `DataclassProxy` registration target - Wrapping the `singledispatch` dispatcher to route dataclasses via `is_dataclass` - Replace legacy `DepRecatEd_symAdaptTensor` with `deprecated_SymAdaptTensor` consistently - Unify `find_components_by_seqaln_rmsfit` logic: - Rename recursive function with underscore prefix - Integrate `merge_small_components` logic into top-level function - Add new options: `pickchain` and `min_chain_atoms` for selective merging - Clean up `merge_small_components` signature and move to main function scope - Add debug logging to `SymmetryManager.__call__` for unsupported types - Improve type hints and cleanup `SymAdapt` class declarations
Added .pre-commit-config.yaml with the following tools: - Commitizen for conventional commit linting - Ruff for linting, formatting, and import sorting - Pre-commit hooks for whitespace, YAML, and merge conflict checks - validate-pyproject for validating pyproject.toml metadata - pytest-pre-commit with `ipd/tests` and parallel execution support
…ss codebase This change updates all occurrences of `ContactMatrixStack` to the more descriptive and consistent `ContactBlockMatrix`, reflecting its role in handling block-wise contact matrices for biological structures. - Renamed class in `contact_matrix.py`, including all docstrings and doctests - Updated all references in documentation, tests, and tutorials accordingly - Changed `contact_matrix_stack()` to `contact_blocks()` for improved clarity - Adjusted SymBody, BodyContacts, and test logic to match new naming - Preserved compatibility with doctests and rich repr output Also included in this commit: - Migrated init-time profiling from custom logic to `evn.chrono_*` API - Refactored `lazy_import.py` to remove pip/mamba install logic and simplify usage - Added `Frags` and `FragsOne2Many` data classes for contact-based fragment grouping - Introduced `npNone` and `NumpyNone` sentinel types for struct field defaults - Improved select logic in `atom_utils` and added `chaincom()` utility - Enhanced `Body` with `token`-based atom indexing methods - Added new test coverage in `test_contact_matrix.py` and `test_sym_builder.py` - Updated dependency bounds for `evn` and pytest in `pyproject.toml`
…ly module
This commit introduces several refactoring changes for consistency and maintainability, improves type hinting and documentation, and temporarily disables the `ipd.atom.assembly` module.
**Refactoring & Standardization:**
* **Metadata Handling:** Introduced `ipd.dev.HoldsMetadata` base class. Classes requiring metadata capabilities (like `Body`, `SymBody`) now inherit from this class, providing `set/get/sync_metadata` methods and a `meta` property explicitly. The `@ipd.dev.holds_metadata` decorator now primarily handles patching `__copy__` to preserve metadata.
* **Dataclass Usage:** Replaced custom `@ipd.struct`/`@ipd.mutablestruct` decorators with `@ipd.dc.dataclass` (likely standard `dataclasses.dataclass`) across classes like `Body`, `SymBody`, and `ContactBlockMatrix`.
* **Dataclass Decorator Wrapping:** Applied `@functools.wraps(dc.dataclass)` to struct decorators in `ipd/_prelude/structs.py` to preserve original class metadata (name, docstring).
* **Mutable Default Fix:** Changed `ipd.npNone` from a direct `dc.field` instance to a function `npNone()` that *returns* a `dc.field`. This ensures each dataclass instance gets a new `NumpyNone` array, preventing issues with shared mutable defaults. (`Body.rescen`, `Body.tokens`).
* **BVH Operation Dispatch:** Modified `_bvh_binary_operation` in `body.py` to accept the operation name as a string (e.g., 'bvh_isect') and use `getattr` for dispatch, instead of passing the function object directly.
**Improvements & Fixes:**
* **`ipd.atom.atom_utils`:**
* Added extensive `typing.overload` definitions for `select` function to improve static type checking based on `chainlist`/`chaindict` arguments.
* Refactored `select` internals for clarity, using explicit casts (`castaa`), `np.isin`, and handling default string args consistently.
* Fixed potential `UFuncNoLoopError` in `join` by using `np.char.add` for string concatenation with numpy arrays.
* **`ipd.atom.body`:**
* Corrected `SymBody.coord` property calculation.
* Improved type hints in `SymBody.contacts`.
* **Namespace:** Added `Observer` to the top-level `ipd` namespace via `ipd/__init__.py`.
**Documentation:**
* **`ipd.observer.observer`:** Added extensive module, class, and method docstrings explaining the Observer pattern implementation (`Subject`, `Observer`, `ObserverMethod`, `hub`).
**Code Disabling:**
* **`ipd.atom.assembly`:** The entire content of this module (including `Assembly`, `AsuSelector`, `NeighborhoodSelector` classes) has been commented out, effectively disabling this functionality.
**Minor:**
* Cleaned up minor formatting and type hints in various files.
* Used `contextlib.suppress` in `ipd.dev.meta.shallow_copy`.
Reviewer's Guide by SourceryThis pull request includes several major changes, including refactoring data classes to use No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR, titled "Crops", reorganizes and extends several modules in the IPD codebase, including updates to atom utility functions, lazy imports, and initialization code while also integrating new modules (frags and crops). Key changes include:
- Refactoring import statements and type annotations in ipd/atom/atom_utils.py (e.g., using "import typing as t").
- Enhancing the overloaded "select" function and updating "join" to use np.char.add.
- Updating prelude modules and the main init.py to use evn.chrono_* functions, as well as minor changes in workflows and configuration files.
Reviewed Changes
Copilot reviewed 127 out of 134 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ipd/atom/atom_utils.py | Refactored import style, updated overloaded functions and added a new "select" implementation |
| ipd/atom/assembly.py | Commented legacy assembly code and maintained changes for potential new usage |
| ipd/atom/init.py | Added new imports for frags and crops modules to support "Crops" feature |
| ipd/_prelude/* | Updated lazy import and structs modules; removed the chrono module |
| ipd/init.py | Replaced ipd_init_checkpoint with evn.chrono_checkpoint calls |
| docs/conf.py, README.md, workflow files | Minor updates to documentation paths and configuration settings |
Files not reviewed (7)
- .python-version: Language not supported
- docs/dev_guide/coding.rst: Language not supported
- docs/dev_guide/dependencies.rst: Language not supported
- docs/tutorials/contact_matrix.rst: Language not supported
- docs/tutorials/symmetry_detection.rst: Language not supported
- docs/tutorials/using_homog.rst: Language not supported
- docs/tutorials/working_with_atoms.rst: Language not supported
Comments suppressed due to low confidence (1)
ipd/atom/atom_utils.py:103
- [nitpick] Consider using a more descriptive and conventional sentinel name (e.g., 'MISSING_STR') for clarity.
missingstr=''
| - name: Deploy to GitHub Pages | ||
| uses: peaceiris/actions-gh-pages@v3 | ||
| if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }} | ||
| if: ${{ github.event_name == 'push' }} # && github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
[nitpick] Check the trailing comment for an extra closing brace '}}' which might be confusing; either remove or clarify the comment.
| if: ${{ github.event_name == 'push' }} # && github.ref == 'refs/heads/main' }} | |
| if: ${{ github.event_name == 'push' }} # Example: Add '&& github.ref == "refs/heads/main"' for main branch only |
penify-dev
bot
added
enhancement
PR Review 🔍
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @willsheffler - I've reviewed your changes - here's some feedback:
Overall Comments:
- This looks like a large change - consider breaking it up into smaller, more manageable pull requests.
- It looks like a lot of code has been commented out - consider removing the dead code.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self._atombvh = hg.SphereBVH_double(self.atoms.coord) # type: ignore | ||
| self._resbvh = hg.SphereBVH_double(self.rescen) # type: ignore | ||
| self.asu = self | ||
| if not self.tokens: self.tokens = self.atoms.res_id |
There was a problem hiding this comment.
suggestion (bug_risk): Clarify tokens initialization condition.
Using 'if not self.tokens' may mistakenly trigger when tokens is an empty array even if not None. Consider explicitly checking for None to avoid unintended reinitialization when a valid (even if empty) tokens array exists.
Suggested implementation:
if self.tokens is None:
self.tokens = self.atoms.res_idEnsure that self.tokens is initialized to None (or not set) before post_init is called, so that the new condition works as expected.
| :alt: partialsum2d illustration | ||
| :alt: partialsum2d illustration | ||
|
|
||
| Illustration of data 2D with pink region to be "summed" and 2D cumulative sum array from which four points are needed to computs the "sum:" ``sum = CSUM[ub1,ub2] (red point) + CSUM[lb1,lb2] (green point) - CUSM[ub1,lb2] (blue point) - CSUM[lb1,lb2] (blue point``. |
There was a problem hiding this comment.
issue (typo): Typo: "computs" should be "compute".
| Illustration of data 2D with pink region to be "summed" and 2D cumulative sum array from which four points are needed to computs the "sum:" ``sum = CSUM[ub1,ub2] (red point) + CSUM[lb1,lb2] (green point) - CUSM[ub1,lb2] (blue point) - CSUM[lb1,lb2] (blue point``. | |
| Illustration of data 2D with pink region to be "summed" and 2D cumulative sum array from which four points are needed to compute the "sum:" ``sum = CSUM[ub1,ub2] (red point) + CSUM[lb1,lb2] (green point) - CUSM[ub1,lb2] (blue point) - CSUM[lb1,lb2] (blue point``. |
| ContactMatrixStack uses a precomputed 2D partialsum array for efficient region-based queries. To explain | ||
| ContactBlockMatrix uses a precomputed 2D partialsum array for efficient region-based queries. To explain | ||
| start with the 1D partialsum case. ``DATA`` is a 1D array and ``SUMS`` is the cumulative sum | ||
| of ``DATA`` (``ipd.partialsum(DATA)``. If we want the sum of ``DATA[i:j]`` we can compute it as |
There was a problem hiding this comment.
issue (typo): Inconsistent backticks around ipd.partialsum(DATA). The second backtick should be single.
| of ``DATA`` (``ipd.partialsum(DATA)``. If we want the sum of ``DATA[i:j]`` we can compute it as | |
| of ``DATA`` (``ipd.partialsum(DATA)`. If we want the sum of ``DATA[i:j]`` we can compute it as |
| if cls.adapts is not None: # type: ignore | ||
|
|
||
| @_sym_adapt.register(cls.adapts) # type: ignore | ||
| if not hasattr(cls, '__adapts__'): |
There was a problem hiding this comment.
issue (complexity): Consider extracting the registration logic into a helper function to simplify subclass definitions and centralize dispatch behavior.
Consider extracting the repeated registration logic from `__init_subclass__` into a small helper so that each subclass only has to specify its adapted types. This reduces inline complexity and centralizes the dispatch‐registration behavior. For example:
```python
def register_adapted_types(cls):
types_to_register = cls.__adapts__ if isinstance(cls.__adapts__, AdaptTypes) else AdaptTypes((cls.__adapts__,))
for adapted_type in types_to_register:
@_sym_adapt.register(adapted_type)
def adapter(thing, sym, isasym=None, cls=cls):
return cls(thing, sym, isasym)
register_adapted_types(cls)Then, in SymAdapt.__init_subclass__, replace the inline registration with a call to this helper. This keeps the subclass definition cleaner while preserving all functionality.
ipd/_prelude/lazy_import.py
Outdated
| if 'doctest' in sys.modules: | ||
| if in_doctest(): | ||
| return FalseModule(self._lazymodule_name if isinstance(self._lazymodule_name, str | ||
| ) else self._lazymodule_name[0]) |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if 'doctest' in sys.modules: | |
| if in_doctest(): | |
| return FalseModule(self._lazymodule_name if isinstance(self._lazymodule_name, str | |
| ) else self._lazymodule_name[0]) | |
| if 'doctest' in sys.modules and in_doctest(): | |
| return FalseModule(self._lazymodule_name if isinstance(self._lazymodule_name, str | |
| ) else self._lazymodule_name[0]) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| contactmat = contactlist.contact_blocks(self.asu.atoms.res_id) | ||
| return contactmat |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| contactmat = contactlist.contact_blocks(self.asu.atoms.res_id) | |
| return contactmat | |
| return contactlist.contact_blocks(self.asu.atoms.res_id) |
| result = (self.partialsum[:, fsz::s, fsz::s] - self.partialsum[:, fsz::s, :-fsz:s] - | ||
| self.partialsum[:, :-fsz:s, fsz::s] + self.partialsum[:, :-fsz:s, :-fsz:s]) | ||
| return result |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| result = (self.partialsum[:, fsz::s, fsz::s] - self.partialsum[:, fsz::s, :-fsz:s] - | |
| self.partialsum[:, :-fsz:s, fsz::s] + self.partialsum[:, :-fsz:s, :-fsz:s]) | |
| return result | |
| return ( | |
| self.partialsum[:, fsz::s, fsz::s] | |
| - self.partialsum[:, fsz::s, :-fsz:s] | |
| - self.partialsum[:, :-fsz:s, fsz::s] | |
| + self.partialsum[:, :-fsz:s, :-fsz:s] | |
| ) |
| print('dumping to:', output) | ||
| output, ext = output.rsplit('.', 1) | ||
| ipd.atom.dump(joint, f'{output}_components.{ext}') | ||
| asu = ipd.atom.Body(joint[joint.chain_id == 'A']) | ||
| sym = ipd.atom.SymBody(asu, ipd.sym.frames('I')) | ||
| sym.dump(f'{output}_icos.{ext}') |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
| @@ -119,7 +119,7 @@ def helper_test_qcpscan_perf(nscan, nres, natom, cyclic, nsamp): | |||
| if nscan**len(lbub) < 1e8: | |||
| with ipd.dev.Timer(verbose=False) as t: | |||
| for isamp in range(nsamp): | |||
There was a problem hiding this comment.
issue (code-quality): Replace unused for index with underscore [×2] (for-index-underscore)
| vox = ipd.cuda.voxel.Voxel(xyz) | ||
|
|
||
| with ipd.dev.Timer(): | ||
| for i in range(10): |
There was a problem hiding this comment.
issue (code-quality): Replace unused for index with underscore [×2] (for-index-underscore)
PR Code Suggestions ✨
|
- Replace direct evn.chrono calls in ipd/__init__.py with the new chronometer API: set `chronometer.name`, emit a profile report on exit, and annotate import/checkpoint scopes - Consolidate imports in ipd/__init__.py (alias `typing` as `t`, import evn for side effects, remove unused `contextlib`) - Remove the legacy `ipd._prelude.lazy_import` module in favor of `evn._prelude.lazy_import` - Update ipd/_prelude/structs and typehints: - Add `import typing as t` and `import dataclasses` - Define `npNone()` and `framesNone()` factory fields - Simplify array‐type classes by inheriting from a common `FalseNDarray` - Fix `SymBody.__getitem__` to handle single-index slicing correctly - Enhance `ipd.dev.storage.package_storage.is_pickle_fname` to recognize `.pkl[.gz|.xz]` and `.pickle[.gz|.xz]` - Instrument `ipd.sym` classes with `@evn.chrono`, add `__bool__` to `SymIndex`, and introduce `null_sym_index`/`null_sym_kind` defaults - Add `NULL` members to `ShapeKind` and `ValueKind` enums - Relax numerical tolerance in `test_thgeom` from `1e-4` to `1e-3` - Remove obsolete `test_lazy_import.py` - Update CI tooling in `citool.py` and its tests to prepend `PYTHONPATH=.:$PYTHONPATH` - Bump project version to `0.4.8` and require `evn>=0.7.8` BREAKING CHANGE: the standalone `ipd._prelude.lazy_import` module has been removed; ensure all lazy imports use `evn.lazyimport`
Description
pymol_symgen.py.oldtest_asugrid.py.__init__.pyto improve performance tracking.qcp_rms.py.Changes walkthrough 📝
pymol_symgen.py
New symmetry generation and visualization functionalitieslegacy/_symfit/pymol_symgen.py
hacky_xtal_makerfunction for crystal structure generation.PymolSymElemclass for symmetry elements in PyMOL.BuildCGOclass for building CGO objects in PyMOL.__init__.py
Refactor initialization and performance trackingipd/init.py
evn.chronofor performance tracking.qcp_rms.py
Fix import path for CUDA RMSipd/cuda/rms/qcp_rms.py
oldtest_asugrid.py
New tests for ASU grid placement functionalitylegacy/_symfit/oldtests/oldtest_asugrid.py
place_asu_gridfunctionality.