Skip to content
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 SyntaxWarning #954

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vieting
Copy link
Contributor

@vieting vieting commented Feb 17, 2022

When running RETURNN, I get

/.../returnn/returnn/tf/network.py:1621: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if loss is 0:
/.../returnn/returnn/tf/util/basic.py:2024: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if dim is not 1:
/.../returnn/returnn/tf/util/basic.py:5550: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if start is 0 and stop is None:
/.../returnn/returnn/tf/layers/base.py:1383: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if c is 0:

I don't see a reason to do it the way it is currently done, so we could just fix it like proposed here.

@vieting vieting requested review from albertz and removed request for albertz February 17, 2022 14:49
@@ -1380,7 +1380,7 @@ def get_constraints_value(self):
c += self.spatial_smoothing * self.get_output_spatial_smoothing_energy()
if self.darc1:
c += self.darc1 * self.get_darc1()
if c is 0:
if c == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong in case c is a tf.Tensor or numpy.ndarray.

Same also for all the other changes.

Copy link
Member

@albertz albertz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not as simple (that's why I did not do it yet).

Basically you need isinstance(c, (int, float, numpy.number)) or so.

@vieting
Copy link
Contributor Author

vieting commented Feb 17, 2022

It's not as simple (that's why I did not do it yet).

Basically you need isinstance(c, (int, float, numpy.number)) or so.

I see. It seems that all tests pass though, so it's maybe difficult to tell what would be correct and doesn't break any setups, right?

@albertz
Copy link
Member

albertz commented Feb 17, 2022

But still it should be done like that. This just shows that the tests don't cover this. I wonder actually, I thought e.g. loss should always become a tf.Tensor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants