This repository has been archived by the owner on Sep 18, 2024. It is now read-only.
Make pylint happy #1649
Merged
Make pylint happy #1649
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liuzhe-lz
commented
Oct 23, 2019
| @@ -191,22 +191,23 @@ def _add_index(in_x, parameter): | |||
| return {NodeType.INDEX: pos, NodeType.VALUE: item} | |||
| else: | |||
| return parameter | |||
| return None # FIXME: what to return here? | |||
There was a problem hiding this comment.
@xuehui1991 Please check this.
There was a problem hiding this comment.
Do not change the logic in this PR.
There was a problem hiding this comment.
Okay. Though it's still smelly to me.
I'll change FIXME into a note.
liuzhe-lz
commented
Oct 23, 2019
| @@ -576,6 +570,7 @@ def to_real_keras_layer(layer): | |||
| return layers.Flatten() | |||
| if is_layer(layer, "GlobalAveragePooling"): | |||
| return layers.GlobalAveragePooling2D() | |||
| return None # FIXME: should here return or raise? | |||
There was a problem hiding this comment.
@xuehui1991 And this.
There was a problem hiding this comment.
do not change the logic.
liuzhe-lz
commented
Oct 23, 2019
| @@ -608,6 +603,7 @@ def is_layer(layer, layer_type): | |||
| return isinstance(layer, (StubFlatten,)) | |||
| elif layer_type == "GlobalAveragePooling": | |||
| return isinstance(layer, StubGlobalPooling) | |||
| return None # FIXME: return or raise? | |||
There was a problem hiding this comment.
return None. do not change the logic, thank you.
xuehui1991
approved these changes
Oct 28, 2019
liuzhe-lz
commented
Oct 28, 2019
| @@ -46,21 +45,20 @@ def trial_end(self, trial_job_id, success): | |||
| trial_job_id: identifier of the trial (str). | |||
| success: True if the trial successfully completed; False if failed or terminated. | |||
| """ | |||
| pass | |||
There was a problem hiding this comment.
@leckie-chn Is this recommended style?
This will cause W0107: Unnecessary pass statement (unnecessary-pass)
There was a problem hiding this comment.
Currently in our pylintrc file W1017 is not enabled. Are you sure you are using the right version of pylintrc file?
There was a problem hiding this comment.
I'm using the rc file included in this PR.
There was a problem hiding this comment.
You should use https://github.com/microsoft/nni/blob/master/pylintrc
|
@liuzhe-lz Just to clarify, which modules is this PR fixing? |
|
With the output on nni SDK with current settings: |
|
@leckie-chn For your pylintrc output:
|
Line 45 in 041c3f0 |
|
@leckie-chn Suggest use |
output w/
|
Merged
|
leckie-chn
approved these changes
Oct 30, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PPO tuner is not touched because I can't understand reported functions. @QuanluZhang
I have not installed all tuner dependencies and disabled
import-errorlocally, so it may be incomplete.