use variable to store the bias of atomic polarizability #4581
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThis pull request enhances the PolarFittingSeA class by renaming and re-declaring the constant_matrix variable as t_constant_matrix within a TensorFlow variable scope, and updating related methods (build and compute_output_stats) to maintain consistency. Additionally, the init_variables method now includes error handling that raises a GraphWithoutTensorError when the tensor cannot be retrieved. In the test suite, a conditional check is introduced to remove a specific key from the data dictionary when the TensorFlow object class starts with "Polar". Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as PolarFittingSeA
participant TF as tf.get_variable
participant G as Graph
U->>P: Call build(input tensors)
P->>TF: Create t_constant_matrix (non-trainable)
TF-->>P: Return t_constant_matrix
U->>P: Call init_variables(graph)
P->>G: Retrieve t_constant_matrix via get_tensor_by_name_from_graph
alt Tensor Found
G-->>P: Return tensor successfully
else Tensor Not Found
P->>P: Raise GraphWithoutTensorError and issue warning
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/fit/polar.py
568-569: SyntaxError: Expected ':', found newline
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (2)
deepmd/tf/fit/polar.py (2)
30-32: LGTM! Import changes are well-organized.The new imports for error handling and tensor retrieval are properly organized and necessary for the functionality changes.
Also applies to: 35-35
437-444: LGTM! Variable scope and initialization are well-implemented.The new variable scope properly initializes
t_bias_atom_polaras a non-trainable variable with appropriate precision and initialization values.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yifan Li李一帆 <yifanl0716@gmail.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/fit/polar.py (1)
573-578: Improve exception chaining.The exception handling should use explicit exception chaining to preserve the traceback information.
Apply this diff to improve the error handling:
try: self.bias_atom_polar = get_tensor_by_name_from_graph( graph, f"fitting_attr{suffix}/t_bias_atom_polar" ) except GraphWithoutTensorError as err: - raise RuntimeError("No bias_atom_polar in the graph.") + raise RuntimeError("No bias_atom_polar in the graph.") from err🧰 Tools
🪛 Ruff (0.8.2)
578-578: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/fit/polar.py
578-578: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Analyze (python)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (4)
deepmd/tf/fit/polar.py (4)
30-32: LGTM!The new import
GraphWithoutTensorErroris correctly added for error handling.
437-444: LGTM!The variable scope and tensor creation for
t_bias_atom_polaris well-implemented:
- Uses appropriate variable scope
- Sets trainable=False as it's a constant bias
- Initializes with the correct shape and data type
469-471: LGTM!The tensor is correctly used in the mixed types case to tile and repeat the bias values.
523-525: LGTM!The tensor is correctly used in the non-mixed types case to add the bias to the diagonal elements.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4581 +/- ##
==========================================
- Coverage 84.61% 84.60% -0.01%
==========================================
Files 680 680
Lines 64467 64476 +9
Branches 3538 3538
==========================================
+ Hits 54547 54553 +6
- Misses 8780 8782 +2
- Partials 1140 1141 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/fit/polar.py (1)
573-579: Add stacklevel to the warning call.The warning about missing mean diagonal terms should include a stacklevel to help users identify the source of the warning.
Apply this diff to improve the warning:
- warnings.warn("You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue.") + warnings.warn( + "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue.", + stacklevel=2 + )🧰 Tools
🪛 Ruff (0.8.2)
578-578: No explicit
stacklevelkeyword argument found(B028)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/fit/polar.py
578-578: No explicit stacklevel keyword argument found
(B028)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test C++ (true)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (3)
deepmd/tf/fit/polar.py (3)
30-36: LGTM! Good error handling imports.The addition of
GraphWithoutTensorErrorandget_tensor_by_name_from_graphimproves error handling when working with TensorFlow graphs.
437-444: LGTM! Well-structured variable initialization.The initialization of
t_bias_atom_polaras a non-trainable TensorFlow variable with proper scope and type is correct. The variable stores the mean of diagonal terms of the polarizability tensor.
469-471: LGTM! Consistent variable usage.The
t_bias_atom_polarvariable is correctly used in both mixed and non-mixed type scenarios for shifting the diagonal terms of the polarizability tensor.Also applies to: 523-525
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deepmd/tf/fit/polar.py (2)
578-580: Add stacklevel to the warning.The warning should include a stacklevel to help users identify the source of the warning in their code.
- warnings.warn( - "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue." - ) + warnings.warn( + "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue.", + stacklevel=2 + )🧰 Tools
🪛 Ruff (0.8.2)
578-578: No explicit
stacklevelkeyword argument found(B028)
581-581: Remove unnecessary pass statement.The
passstatement is not needed here as the try-except block already has a warning statement.- pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/fit/polar.py
578-578: No explicit stacklevel keyword argument found
(B028)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Analyze (python)
- GitHub Check: Test C++ (false)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test C++ (true)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
🔇 Additional comments (3)
deepmd/tf/fit/polar.py (3)
30-36: LGTM!The new imports for error handling and tensor retrieval are well-organized and necessary for the enhanced functionality.
437-444: LGTM!The variable scope and tensor creation follow TensorFlow best practices:
- Proper scope naming
- Correct variable initialization
- Appropriate trainable flag
469-471: LGTM!The changes correctly use the new
t_bias_atom_polartensor variable for bias calculations.Also applies to: 523-525
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deepmd/tf/fit/polar.py (2)
469-471: Consider extracting tensor operations into helper functions.The tensor operations for bias application are duplicated between mixed and non-mixed types modes. Consider extracting these into helper functions for better maintainability.
+ def _apply_bias_tensor(self, tensor, sel_type_idx=None, batch_shape=None): + """Apply bias tensor with proper slicing and broadcasting.""" + if sel_type_idx is not None: + bias = tf.slice(self.t_bias_atom_polar, [sel_type_idx], [1]) + else: + bias = self.t_bias_atom_polar + return tensor + bias * tf.eye( + 3, + batch_shape=batch_shape or [1, 1], + dtype=GLOBAL_TF_FLOAT_PRECISION, + ) # In mixed_types mode - final_layer += tf.expand_dims( - tf.expand_dims(constant_matrix, -1), -1 - ) * tf.eye(3, batch_shape=[1, 1], dtype=GLOBAL_TF_FLOAT_PRECISION) + final_layer = self._apply_bias_tensor(final_layer) # In non-mixed_types mode - final_layer = final_layer + tf.slice( - self.t_bias_atom_polar, [sel_type_idx], [1] - ) * tf.eye( - 3, - batch_shape=[tf.shape(inputs)[0], natoms[2 + type_i]], - dtype=GLOBAL_TF_FLOAT_PRECISION, - ) + final_layer = self._apply_bias_tensor( + final_layer, + sel_type_idx=sel_type_idx, + batch_shape=[tf.shape(inputs)[0], natoms[2 + type_i]] + )Also applies to: 523-525
572-580: Add stacklevel to warning and consider adding version info.Good error handling, but a few improvements are suggested:
- Add stacklevel to the warning to help users locate the source
- Consider including the version where this change was introduced in the warning message
warnings.warn( - "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue." + "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with DeePMD-kit v2.2.0 or later to avoid this issue.", + stacklevel=2 )🧰 Tools
🪛 Ruff (0.8.2)
578-578: No explicit
stacklevelkeyword argument found(B028)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
deepmd/tf/fit/polar.py
578-578: No explicit stacklevel keyword argument found
(B028)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
🔇 Additional comments (2)
deepmd/tf/fit/polar.py (2)
30-36: LGTM! Clean import additions.The new imports for error handling and tensor retrieval are well-organized and appropriately scoped.
437-444: LGTM! Well-structured tensor initialization.The variable scope and tensor initialization follow TensorFlow best practices. The tensor is correctly set as non-trainable and uses the appropriate precision.
Let's verify the tensor initialization:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if t_bias_atom_polar is used consistently across the codebase # Search for tensor usage rg "t_bias_atom_polar" -A 3Length of output: 1181
LGTM! Consistent usage of t_bias_atom_polar verified.
The regex search confirms that the variable is initialized as a non-trainable tensor within its designated variable scope in deepmd/tf/fit/polar.py and is referenced consistently in subsequent operations (e.g., within tf.repeat and indexed calls). No inconsistencies or misuses were found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/fit/polar.py (1)
573-581: Enhance the warning message clarity.While the error handling is well-implemented, the warning message could be more specific about the impact of missing bias values.
Consider updating the warning message to be more explicit:
- "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to wrong inference results. You may train your model with the latest DeePMD-kit to avoid this issue.", + "You are trying to read a model trained with shift_diag=True, but the mean of the diagonal terms of the polarizability is not stored in the graph. This will lead to incorrect polarizability predictions as the bias values will default to zero. Please retrain your model with the latest DeePMD-kit to include the required bias values.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(5 hunks)
🔇 Additional comments (3)
deepmd/tf/fit/polar.py (3)
30-32: LGTM! Clean import additions.The new imports for error handling and tensor retrieval are well-organized and follow Python import style guidelines.
Also applies to: 35-35
437-444: LGTM! Well-structured tensor initialization.The variable scope and tensor initialization are correctly implemented:
- Proper variable scope naming with suffix support
- Non-trainable tensor for bias values
- Correct shape and type initialization
469-471: LGTM! Proper tensor operations.The changes correctly replace direct matrix operations with TensorFlow tensor operations, maintaining the same functionality while being more TensorFlow-idiomatic.
Also applies to: 523-525
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/fit/polar.py (1)
643-645: Add safety check for bias_atom_polar during serialization.The serialization logic should handle cases where
bias_atom_polarmight be None, which could happen ifinit_variableswasn't called or encountered an error.Apply this diff to add a safety check:
"@variables": { - "bias_atom_polar": self.bias_atom_polar.reshape(-1), + "bias_atom_polar": self.bias_atom_polar.reshape(-1) if hasattr(self, 'bias_atom_polar') else None, },And in deserialize:
- fitting.bias_atom_polar = data["@variables"]["bias_atom_polar"].ravel() + bias_atom_polar = data["@variables"].get("bias_atom_polar") + if bias_atom_polar is not None: + fitting.bias_atom_polar = bias_atom_polar.ravel()Also applies to: 673-673
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deepmd/tf/fit/polar.py(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C++ (cpu, cpu)
🔇 Additional comments (4)
deepmd/tf/fit/polar.py (4)
30-36: LGTM!The new imports enhance error handling and tensor management capabilities.
437-444: LGTM!The variable scope and initialization follow TensorFlow best practices. The variable is correctly marked as non-trainable and initialized with the appropriate shape and type.
469-471: LGTM!The variable is used consistently across both mixed and non-mixed type modes, maintaining the same functionality while improving tensor management.
Also applies to: 523-525
573-581: LGTM!The error handling is robust and user-friendly. The warning message clearly explains the issue and provides guidance for resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/fit/polar.py (1)
643-651: Verify the debug print statement.The
print("executing deserialize")statement on line 670 appears to be a debug statement that should be removed.- print("executing deserialize")Also applies to: 670-670, 680-680
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/dpmodel/fitting/polarizability_fitting.py(1 hunks)deepmd/tf/fit/polar.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- deepmd/dpmodel/fitting/polarizability_fitting.py
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (4)
deepmd/tf/fit/polar.py (4)
30-36: LGTM! Clean import organization.The new imports for error handling (
GraphWithoutTensorError) and tensor retrieval (get_tensor_by_name_from_graph) are well-organized and properly scoped.
437-444: LGTM! Proper tensor variable initialization.The conversion of
constant_matrixto a TensorFlow variablet_constant_matrixis well-implemented:
- Uses proper variable scope
- Correctly marked as non-trainable
- Maintains the original shape and precision
469-471: LGTM! Consistent tensor usage.The code consistently uses the new
t_constant_matrixtensor variable throughout the implementation, both in the mixed and non-mixed type scenarios.Also applies to: 523-525
572-581: LGTM! Robust error handling for tensor retrieval.The implementation properly handles the case where the polarizability bias tensor is missing from the graph:
- Uses try-except to catch
GraphWithoutTensorError- Provides a clear warning message explaining the impact
- Suggests a solution to avoid the issue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
source/tests/consistent/common.py (1)
357-359: LGTM! Consider adding a comment explaining the bias removal.The code correctly removes the bias_atom_e from variables when dealing with Polar classes. However, adding a comment would help explain why this special handling is needed.
if tf_obj.__class__.__name__.startswith("Polar"): + # Remove bias_atom_e from variables as it's handled differently between TF and PT data1["@variables"].pop("bias_atom_e")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/tests/consistent/common.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Analyze (python)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (c-cpp)
…#4581) This PR fixes deepmodeling#4559. It uses variable to store the bias of atomic polarizability, which is the mean of the diagonal terms of the polarizability tensor. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Enhanced core processing reliability with improved error management and resource consistency, leading to more stable model performance during inference. - Tests - Refined internal validation checks to better support advanced configurations and ensure consistent outcomes. These improvements contribute to a more robust and reliable experience for end-users. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yifan Li李一帆 <yifanl0716@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 3ad94ff)
This PR fixes #4559.
It uses variable to store the bias of atomic polarizability, which is the mean of the diagonal terms of the polarizability tensor.
Summary by CodeRabbit
These improvements contribute to a more robust and reliable experience for end-users.