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 warnings about code quality #97

Open
egpbos opened this issue May 18, 2021 · 0 comments
Open

Fix warnings about code quality #97

egpbos opened this issue May 18, 2021 · 0 comments

Comments

@egpbos
Copy link
Collaborator

egpbos commented May 18, 2021

Flake8 after #91 still gives a few code quality warnings that need a moment of thought to "fix". These are the following (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.

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

No branches or pull requests

1 participant