-
Notifications
You must be signed in to change notification settings - Fork 184
merge devel to master and release v0.13.2 #1789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.6 → v0.9.7](astral-sh/ruff-pre-commit@v0.9.6...v0.9.7) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.7 → v0.9.9](astral-sh/ruff-pre-commit@v0.9.7...v0.9.9) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.9 → v0.9.10](astral-sh/ruff-pre-commit@v0.9.9...v0.9.10) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.10 → v0.11.0](astral-sh/ruff-pre-commit@v0.9.10...v0.11.0) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.0 → v0.11.2](astral-sh/ruff-pre-commit@v0.11.0...v0.11.2) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.2 → v0.11.4](astral-sh/ruff-pre-commit@v0.11.2...v0.11.4) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.11.5](astral-sh/ruff-pre-commit@v0.11.4...v0.11.5) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.5 → v0.11.6](astral-sh/ruff-pre-commit@v0.11.5...v0.11.6) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.6 → v0.11.7](astral-sh/ruff-pre-commit@v0.11.6...v0.11.7) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.7 → v0.11.8](astral-sh/ruff-pre-commit@v0.11.7...v0.11.8) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…eepmodeling#1748) Fix deepmodeling#1745 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved the processing of molecular dynamics data directories to ensure all relevant perturbation indices are included during data collection. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.8 → v0.11.9](astral-sh/ruff-pre-commit@v0.11.8...v0.11.9) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
fix the inconsistent lengths between `sys_batch_size` and `sys_configs` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Cleaned up configuration by removing redundant entries from the batch size settings in the JSON file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Yong-Bin Zhuang <38876805+robinzyb@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.9 → v0.11.10](astral-sh/ruff-pre-commit@v0.11.9...v0.11.10) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved the selection of pseudopotential and orbital files to ensure only relevant files for the atoms present in the system are included. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: root <pxlxingliang> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
The line I removed seems to be unused. xref: 1c18b1c <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved consistency in reading model deviation data by using a unified helper function for file processing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.10 → v0.11.13](astral-sh/ruff-pre-commit@v0.11.10...v0.11.13) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.11.13 → v0.12.0](astral-sh/ruff-pre-commit@v0.11.13...v0.12.0) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.0 → v0.12.1](astral-sh/ruff-pre-commit@v0.12.0...v0.12.1) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.1 → v0.12.2](astral-sh/ruff-pre-commit@v0.12.1...v0.12.2) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.2 → v0.12.3](astral-sh/ruff-pre-commit@v0.12.2...v0.12.3) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.3 → v0.12.4](astral-sh/ruff-pre-commit@v0.12.3...v0.12.4) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.4 → v0.12.5](astral-sh/ruff-pre-commit@v0.12.4...v0.12.5) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
<!--pre-commit.ci start--> updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.5 → v0.12.7](astral-sh/ruff-pre-commit@v0.12.5...v0.12.7) <!--pre-commit.ci end--> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
numpy.random.Generator.shuffle() performs the intended operation of shuffling the order of atoms. numpy.random.Generator.permuted() permutes the elements along the given axis, which mixes the coordinates of all atoms shuffle  permuted  <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved the process that randomizes system coordinates when a specific option is enabled, enhancing internal data handling and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: ztzkz <95387041+ztzkz@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis update introduces stricter mapping and validation for pseudopotential and orbital files in ABACUS SCF structure generation, adds automatic multiplicity calculation for CP2K input, adjusts coordinate shuffling and data aggregation logic in the generator, updates a pre-commit hook version, removes redundant JSON entries, and makes a minor test adjustment. Changes
Sequence Diagram(s)Automatic Multiplicity Calculation in CP2K Input GenerationsequenceDiagram
participant User
participant make_cp2k_input
participant calculate_multiplicity
User->>make_cp2k_input: Provide sys_data, fp_params
make_cp2k_input->>make_cp2k_input: Extract atom_names, atom_types, charge
alt multiplicity in fp_params
make_cp2k_input->>make_cp2k_input: Use provided multiplicity
else
make_cp2k_input->>calculate_multiplicity: atom_names, atom_types, charge
calculate_multiplicity-->>make_cp2k_input: computed multiplicity
end
make_cp2k_input->>make_cp2k_input: Merge multiplicity into input dict
make_cp2k_input-->>User: Return CP2K input string
ABACUS SCF Structure Generation with Type MappingsequenceDiagram
participant User
participant make_abacus_scf_stru
User->>make_abacus_scf_stru: Provide sys_data, fp_pp_files, fp_orb_files, type_map
make_abacus_scf_stru->>make_abacus_scf_stru: Validate type_map matches sys_data atom names
make_abacus_scf_stru->>make_abacus_scf_stru: Check lengths of file lists and type_map
make_abacus_scf_stru->>make_abacus_scf_stru: Map files to atom types in sys_data order
make_abacus_scf_stru-->>User: Return structured data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dpgen/generator/lib/cp2k.py (1)
246-284
: Document the limitation of the multiplicity heuristic.The function uses a simplified heuristic (even electrons = singlet, odd = doublet) which may not be appropriate for all chemical systems. Many transition metal complexes, radicals, and other systems can have triplet, quartet, or higher multiplicities even with specific electron counts.
Consider adding a more prominent warning in the docstring:
def calculate_multiplicity(atom_names, atom_types, charge=0): """ Calculate the multiplicity based on atom species, quantities, and system charge. + **WARNING: This is a basic heuristic that may not be suitable for complex systems.** + This function provides a basic heuristic for determining multiplicity: - Even number of electrons -> singlet (multiplicity = 1) - Odd number of electrons -> doublet (multiplicity = 2) - Note: This approach assumes that an odd electron count always results in a doublet state. - It does not account for systems with multiple unpaired electrons, which can have higher - multiplicities (e.g., triplet, quartet, etc.). Users should be aware of this limitation - and use the function accordingly. + **IMPORTANT LIMITATIONS:** + - Does not account for high-spin vs low-spin states in transition metals + - Cannot predict triplet ground states (e.g., O2, carbenes) + - Ignores magnetic coupling in multi-metal systems + - Users should verify the calculated multiplicity for their specific system
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yaml
(1 hunks)dpgen/data/gen.py
(1 hunks)dpgen/generator/lib/abacus_scf.py
(1 hunks)dpgen/generator/lib/cp2k.py
(3 hunks)dpgen/generator/run.py
(2 hunks)examples/run/dp2.x-lammps-cp2k/methane/param-ch4.json
(0 hunks)tests/data/test_gen_bulk_abacus.py
(0 hunks)
💤 Files with no reviewable changes (2)
- tests/data/test_gen_bulk_abacus.py
- examples/run/dp2.x-lammps-cp2k/methane/param-ch4.json
⏰ 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). (2)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
🔇 Additional comments (7)
.pre-commit-config.yaml (1)
31-31
: Confirm ruff-pre-commit v0.12.7 compatibilityBased on the changelog, the hook has upgraded its dependencies (notably Pydantic 1→2) and improved file resolution/inclusion/exclusion logic. While no immediately breaking issues are documented, please:
- Ensure that
rev: v0.12.7
is a valid tag in theruff-pre-commit
repo.- Run
pre-commit install && pre-commit run --all-files
locally to catch any new lint errors or config mismatches.- Review your
.pre-commit-config.yaml
for any deprecated or renamed settings (e.g. patterns underexclude
/include
).If everything passes, no further changes are needed here; otherwise, adjust your configuration to match the updated hook behavior.
dpgen/generator/lib/abacus_scf.py (1)
216-247
: Excellent improvement in file mapping validation and consistency.The changes enhance robustness by:
- Adding proper validation for
type_map
parameter- Ensuring all atom names are mapped correctly to their pseudopotential and orbital files
- Providing clear error messages for mismatched configurations
- Using explicit indexing based on
type_map
instead of simple orderingThis prevents silent errors and improves data integrity in ABACUS structure generation.
dpgen/generator/lib/cp2k.py (2)
3-122
: LGTM! Comprehensive atomic numbers dictionary.The dictionary covers all elements from H to Og (atomic numbers 1-118) and will support multiplicity calculations for all known elements.
297-317
: Good integration of automatic multiplicity calculation.The implementation properly:
- Extracts necessary system data (atom_names, atom_types, charge)
- Uses user-provided multiplicity when available
- Falls back to automatic calculation when needed
- Integrates the result into the CP2K configuration
dpgen/generator/run.py (3)
1308-1308
: Verify behavioral change from permuted to shuffle is intentionalThis change modifies the behavior from creating a new shuffled array (
rng.permuted
) to modifying the existing array in-place (rng.shuffle
). This could potentially affect other parts of the code that rely on the originalsystem.data["coords"]
remaining unchanged after this operation.Please confirm this behavioral change is intentional and that no other code depends on the original coordinates remaining unmodified.
2480-2482
: LGTM: Good refactoring to centralize model deviation file readingThe use of
_read_model_devi_file
helper function instead of directnp.loadtxt
improves code consistency and ensures standardized handling of model deviation data with proper parameters for averaging and merging trajectories.
4526-4528
: LGTM: Good defensive programming to prevent empty systemsThis change ensures that only systems containing data are appended to
all_sys
and that the countericount
accurately reflects the number of successfully parsed systems. This prevents empty systems from corrupting the aggregated dataset and improves data integrity.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests