-
Notifications
You must be signed in to change notification settings - Fork 520
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
fix(tf): prevent fitting_attr
variable scope from becoming fitting_attr_1
#3930
Conversation
Fix deepmodeling#3928. Prevent `fitting_attr` from becoming `fitting_attr_1`. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
WalkthroughThe recent changes focus on modifying the Changes
Sequence Diagram(s)[Not applicable: Changes involve simple substitutions in parameter values for existing function calls and do not modify control flow or functionalities substantially.] Assessment against linked issues
Tip Early access features
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Outside diff range and nitpick comments (10)
deepmd/tf/descriptor/se_a_mask.py (5)
Line range hint
124-124
: Avoid using mutable data structures for argument defaults.Replace the default value of
exclude_types
withNone
and initialize it within the function to avoid potential issues with mutable default arguments.exclude_types: Optional[List[List[int]]] = None,And initialize within the function:
if exclude_types is None: exclude_types = []Tools
Ruff
315-315: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
129-129
: Avoid using mutable data structures for argument defaults.Replace the default value of
layer_name
withNone
and initialize it within the function to avoid potential issues with mutable default arguments.layer_name: Optional[List[Optional[str]]] = None,And initialize within the function:
if layer_name is None: layer_name = []Tools
Ruff
315-315: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
185-188
: Remove unused local variables.The variables
avg_zero
andstd_ones
are assigned but never used.- avg_zero = np.zeros([self.ntypes, self.ndescrpt]).astype(GLOBAL_NP_FLOAT_PRECISION) - std_ones = np.ones([self.ntypes, self.ndescrpt]).astype(GLOBAL_NP_FLOAT_PRECISION)Tools
Ruff
315-315: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
225-225
: Add explicitstacklevel
keyword argument to warnings.warn.To ensure the warning points to the correct location in the code, add the
stacklevel
argument.warnings.warn("The cutoff radius is not used for this descriptor", stacklevel=2)Tools
Ruff
315-315: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
324-331
: Remove unused local variables.The variables
t_rcut
,t_ntypes
,t_ndescrpt
, andt_sel
are assigned but never used.- t_rcut = tf.constant(self.rcut, name="rcut", dtype=GLOBAL_TF_FLOAT_PRECISION) - t_ntypes = tf.constant(self.ntypes, name="ntypes", dtype=tf.int32) - t_ndescrpt = tf.constant(self.ndescrpt, name="ndescrpt", dtype=tf.int32) - t_sel = tf.constant(self.sel_a, name="sel", dtype=tf.int32)Tools
Ruff
315-315: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
deepmd/tf/fit/dos.py (4)
Line range hint
111-111
: Avoid using mutable data structures for argument defaults.Replace the default value of
neuron
withNone
and initialize it within the function to avoid potential issues with mutable default arguments.neuron: Optional[List[int]] = None,And initialize within the function:
if neuron is None: neuron = [120, 120, 120]Tools
Ruff
443-443: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
444-444: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
445-445: Local variable
t_numb_dos
is assigned to but never usedRemove assignment to unused variable
t_numb_dos
(F841)
Line range hint
325-328
: Use ternary operator instead ofif
-else
-block.Replace the
if
-else
block with a ternary operator for conciseness.one_layer = one_layer_nvnmd if nvnmd_cfg.enable else one_layer_deepmdTools
Ruff
443-443: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
444-444: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
445-445: Local variable
t_numb_dos
is assigned to but never usedRemove assignment to unused variable
t_numb_dos
(F841)
Line range hint
497-498
: Combine nestedif
statements usingand
.Combine the
if
statements for conciseness.if self.layer_name is not None and self.layer_name[ii] is not None:Tools
Ruff
443-443: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
444-444: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
445-445: Local variable
t_numb_dos
is assigned to but never usedRemove assignment to unused variable
t_numb_dos
(F841)
Line range hint
623-629
: Usecontextlib.suppress
instead oftry
-except
-pass
.Use
contextlib.suppress
for cleaner exception handling.from contextlib import suppress with suppress(GraphWithoutTensorError): self.bias_dos = get_tensor_by_name_from_graph( graph, f"fitting_attr{suffix}/t_bias_dos" )Tools
Ruff
443-443: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
444-444: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
445-445: Local variable
t_numb_dos
is assigned to but never usedRemove assignment to unused variable
t_numb_dos
(F841)
deepmd/tf/fit/ener.py (1)
Line range hint
160-160
: Avoid mutable data structures for argument defaults.Replace mutable data structures with
None
and initialize within the function.- neuron: List[int] = [120, 120, 120], - atom_ener: List[float] = [], + neuron: Optional[List[int]] = None, + atom_ener: Optional[List[float]] = None,self.n_neuron = neuron if neuron is not None else [120, 120, 120] self.atom_ener_v = atom_ener if atom_ener is not None else []Also applies to: 168-168
Tools
Ruff
537-537: Use
key in dict
instead ofkey in dict.keys()
Remove
.keys()
(SIM118)
541-541: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
542-542: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
This reverts commit 39bd6be.
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
fitting_attr
variable scope from becoming fitting_attr_1
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: 3
Outside diff range and nitpick comments (5)
deepmd/tf/descriptor/se_a_mask.py (1)
Line range hint
325-332
: Remove unused local variables.The variables
t_rcut
,t_ntypes
,t_ndescrpt
, andt_sel
are assigned but never used.- t_rcut = tf.constant( - self.rcut, - name="rcut", - dtype=GLOBAL_TF_FLOAT_PRECISION, - ) - t_ntypes = tf.constant(self.ntypes, name="ntypes", dtype=tf.int32) - t_ndescrpt = tf.constant(self.ndescrpt, name="ndescrpt", dtype=tf.int32) - t_sel = tf.constant(self.sel_a, name="sel", dtype=tf.int32)Tools
Ruff
316-316: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
deepmd/tf/fit/ener.py (4)
Line range hint
535-537
: Simplify nested if statements.Combine the nested if statements into a single if statement.
- if "t_bias_atom_e" in nvnmd_cfg.weight.keys(): + if "t_bias_atom_e" in nvnmd_cfg.weight:Tools
Ruff
537-537: Use
key in dict
instead ofkey in dict.keys()
Remove
.keys()
(SIM118)
542-542: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
543-543: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
Line range hint
607-608
: Simplify nested if statements.Combine the nested if statements using
and
.- if ( - nvnmd_cfg.enable - and nvnmd_cfg.quantize_descriptor - and nvnmd_cfg.restore_descriptor - and (nvnmd_cfg.version == 1) - ): + if nvnmd_cfg.enable and nvnmd_cfg.quantize_descriptor and nvnmd_cfg.restore_descriptor and (nvnmd_cfg.version == 1):Tools
Ruff
537-537: Use
key in dict
instead ofkey in dict.keys()
Remove
.keys()
(SIM118)
542-542: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
543-543: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
Line range hint
398-401
: Use ternary operator.Replace the if-else block with a ternary operator.
- if nvnmd_cfg.enable: - one_layer = one_layer_nvnmd - else: - one_layer = one_layer_deepmd + one_layer = one_layer_nvnmd if nvnmd_cfg.enable else one_layer_deepmdTools
Ruff
537-537: Use
key in dict
instead ofkey in dict.keys()
Remove
.keys()
(SIM118)
542-542: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
543-543: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
Line range hint
1034-1037
: Use ternary operator.Replace the if-else block with a ternary operator.
- if sys.pbc: - box = test_data["box"][:numb_test] - else: - box = None + box = test_data["box"][:numb_test] if sys.pbc else NoneTools
Ruff
537-537: Use
key in dict
instead ofkey in dict.keys()
Remove
.keys()
(SIM118)
542-542: Local variable
t_dfparam
is assigned to but never usedRemove assignment to unused variable
t_dfparam
(F841)
543-543: Local variable
t_daparam
is assigned to but never usedRemove assignment to unused variable
t_daparam
(F841)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #3930 +/- ##
==========================================
- Coverage 82.88% 82.87% -0.01%
==========================================
Files 520 520
Lines 50679 50678 -1
Branches 3015 3015
==========================================
- Hits 42003 42001 -2
+ Misses 7740 7738 -2
- Partials 936 939 +3 ☔ View full report in Codecov by Sentry. |
It looks something goes wrong in the UT... |
This reverts commit 2ccaa22.
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
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: 2
Outside diff range and nitpick comments (4)
deepmd/tf/descriptor/se_a_mask.py (4)
Line range hint
325-325
: Remove unused local variable.The variable
t_rcut
is assigned but never used.- t_rcut = tf.constant( - self.rcut, - name="rcut", - dtype=GLOBAL_TF_FLOAT_PRECISION, - )Tools
Ruff
314-314: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
330-330
: Remove unused local variable.The variable
t_ntypes
is assigned but never used.- t_ntypes = tf.constant(self.ntypes, name="ntypes", dtype=tf.int32)
Tools
Ruff
314-314: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
331-331
: Remove unused local variable.The variable
t_ndescrpt
is assigned but never used.- t_ndescrpt = tf.constant(self.ndescrpt, name="ndescrpt", dtype=tf.int32)
Tools
Ruff
314-314: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
Line range hint
332-332
: Remove unused local variable.The variable
t_sel
is assigned but never used.- t_sel = tf.constant(self.sel_a, name="sel", dtype=tf.int32)
Tools
Ruff
314-314: Local variable
t_aparam_nall
is assigned to but never usedRemove assignment to unused variable
t_aparam_nall
(F841)
@@ -311,8 +311,9 @@ | |||
aparam[:, :] is the real/virtual sign for each atom. | |||
""" | |||
aparam = input_dict["aparam"] | |||
with tf.variable_scope("fitting_attr" + suffix, reuse=reuse): | |||
t_aparam_nall = tf.constant(True, name="aparam_nall", dtype=tf.bool) | |||
t_aparam_nall = tf.constant( |
Check notice
Code scanning / CodeQL
Unused local variable Note
fitting_attr
variable scope from becoming fitting_attr_1
fitting_attr
variable scope from becoming fitting_attr_1
…_attr_1` (deepmodeling#3930) Fix deepmodeling#3928. Prevent `fitting_attr` from becoming `fitting_attr_1`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved TensorFlow variable scope management by switching to `tf.AUTO_REUSE` to streamline code and reduce the likelihood of variable reuse conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu> (cherry picked from commit e809e64)
…_attr_1` (#3930) Fix #3928. Prevent `fitting_attr` from becoming `fitting_attr_1`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved TensorFlow variable scope management by switching to `tf.AUTO_REUSE` to streamline code and reduce the likelihood of variable reuse conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu> (cherry picked from commit e809e64)
…_attr_1` (deepmodeling#3930) Fix deepmodeling#3928. Prevent `fitting_attr` from becoming `fitting_attr_1`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved TensorFlow variable scope management by switching to `tf.AUTO_REUSE` to streamline code and reduce the likelihood of variable reuse conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Fix #3928. Prevent
fitting_attr
from becomingfitting_attr_1
.Summary by CodeRabbit
tf.AUTO_REUSE
to streamline code and reduce the likelihood of variable reuse conflicts.