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 flake8 layout warnings #91

Merged
merged 1 commit into from
May 18, 2021
Merged

fix flake8 layout warnings #91

merged 1 commit into from
May 18, 2021

Conversation

egpbos
Copy link
Collaborator

@egpbos egpbos commented May 17, 2021

This PR fixes most warnings that flake8 gives.

A few notable warnings remain that require some further discussion (grouped by type):

./platalea/vq.py:49:12: E741 ambiguous variable name 'l'
./platalea/encoders.py:76:13: E741 ambiguous variable name 'l'
./platalea/encoders.py:134:13: E741 ambiguous variable name 'l'
./platalea/encoders.py:270:13: E741 ambiguous variable name 'l'
./platalea/encoders.py:354:13: E741 ambiguous variable name 'l'
./platalea/encoders.py:411:17: E741 ambiguous variable name 'l'
./platalea/encoders.py:446:17: E741 ambiguous variable name 'l'
./platalea/encoders.py:489:17: E741 ambiguous variable name 'l'
./platalea/utils/preprocessing.py:109:9: E741 ambiguous variable name 'l'
./platalea/utils/flickr8k_filter_metadata.py:7:5: E741 ambiguous variable name 'I'

./platalea/decoders.py:124:5: C901 'TextDecoder.beam_search' is too complex (16)
./platalea/xer.py:51:1: C901 'wer_sent' is too complex (20)
./platalea/asr.py:83:1: C901 'experiment' is too complex (14)

./platalea/utils/evaluate_net.py:27:9: E731 do not assign a lambda expression, use a def

The ambiguous variable names should be easy to fix, but I'm not sure, so we should check.

The complexity warnings usually mean that it may make sense to refactor a function into some separate functions. The best time to do this, I think, would be when you want to work on the function for some other reason. Otherwise, if it works, maybe better to leave it alone.

The assignment of lambda expressions makes some sense (you can look up the PEP8 explanation), but I'm not 100% convinced that you should use full def's just to set some default parameter, which is what it's used for here. However, it does maybe indicate a slight code smell: it may be cleaner to just pass around this beam_size parameter in a parameter dict so that the caller of this function can just pass that stuff to the function when they call it. Something to consider.

@egpbos egpbos mentioned this pull request May 18, 2021
5 tasks
Copy link
Contributor

@cwmeijer cwmeijer left a comment

Choose a reason for hiding this comment

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

Mostly shifting around digital white spaces. Excellent job. 😝

Nice to see a few less linter messages. 👍

for f in range (0, nframes):
frame = data[f * frame_shift : f * frame_shift + window_size]
for f in range(0, nframes):
frame = data[(f * frame_shift):(f * frame_shift + window_size)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is interesting to me. Is it better with brackets? Is that a personal thing or did some linter suggest this?
It's not that I disagree; either way is fine with me, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it was interesting. The linter complained about the space in front of the colon. Not about the one behind the colon, strangely. However, when you write it without the parentheses, it becomes hard to read, because the colon is hard to notice, so it looks more like a single index with a long, complex calculation, than a range-index. I think the underlying issue here is that for readability it would be better to split into three lines: begin = f * frame_shift, end = f * frame_shift + window_size and frame = data[begin:end]. But not hugely important, I would say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or actually just data[begin:begin+window_size].

@egpbos egpbos merged commit 3800db6 into master May 18, 2021
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.

None yet

2 participants