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 Pylint warnings #83

Merged
merged 13 commits into from
Nov 13, 2023
Merged

Fix Pylint warnings #83

merged 13 commits into from
Nov 13, 2023

Conversation

cofri
Copy link
Collaborator

@cofri cofri commented Oct 25, 2023

Pylint returns relevant warnings which are not raised by flake8, the linter used in our CI process. This PR aims at fixing some of these errors to improve deel-lip base code. Each commit of this PR deals with one message type, given in the commit message.

Descriptions for all Pylint messages can be found here.

@cofri cofri marked this pull request as draft October 25, 2023 15:39
@cofri cofri marked this pull request as ready for review October 25, 2023 17:28
@cofri cofri self-assigned this Oct 27, 2023
Copy link
Member

Choose a reason for hiding this comment

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

We need to check if the argument training can be removed. ( especially when calling model(x, training=True) )
(this behavior might be dependent on the tf version)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep **kwargs in the call when training is not used?

Copy link
Member

@thib-s thib-s left a comment

Choose a reason for hiding this comment

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

Great work, we need to discuss the previous comment, otherwise LGTM

Copy link
Member

@y-prudent y-prudent left a comment

Choose a reason for hiding this comment

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

Same remark as Thibaut, once the issue (?) is solved LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep **kwargs in the call when training is not used?

Copy link
Member

@thib-s thib-s left a comment

Choose a reason for hiding this comment

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

I've been looking at the code of official keras layers, the parameter training seems not mandatory when the layer has the same behavior in train and test.

Copy link
Collaborator

@franckma31 franckma31 left a comment

Choose a reason for hiding this comment

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

fine for me

@thib-s thib-s merged commit de7c524 into master Nov 13, 2023
5 checks passed
@thib-s thib-s deleted the fix_pylint_warnings branch November 13, 2023 16:15
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.

4 participants