arrayfire/arrayfire-pythonPublic

NotificationsYou must be signed in to change notification settings
Fork64
Star419

Flake8 code style fixes#241

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

roaffix wants to merge1 commit intoarrayfire:devfromroaffix:fix/flake8-refactoring

Closed

Flake8 code style fixes#241

roaffix wants to merge1 commit intoarrayfire:devfromroaffix:fix/flake8-refactoring

Conversation

Copy link

Contributor

roaffix commentedNov 13, 2020

Fix flake8 errors
Fix some FIXME tags
Moveto_str method to library from utils to avoid import errors

roaffix changed the base branch frommaster todev

November 13, 2020 04:54

roaffix changed the title~~Flake8 code stype fixes~~Flake8 code style fixes

Nov 13, 2020

roaffix force-pushed thefix/flake8-refactoring branch from0d9164c to04d50ceCompare

November 13, 2020 04:57

9prady9 reviewed

Nov 13, 2020

View reviewed changes

Copy link

Member

9prady9 left a comment

Choose a reason for hiding this comment

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

Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:

if <var> is None: orif <var> is not None should be preferred overif <var>: per PEP8 guidline -https://www.python.org/dev/peps/pep-0008/#programming-recommendations
if <var> == 0 or <some number> - not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead ofif <var>: which seems like checking for None and may lead to confusion.

arrayfire/library.py OutdatedShow resolvedHide resolved

roaffix force-pushed thefix/flake8-refactoring branch 2 times, most recently from16f3c01 to9b328f1Compare

November 13, 2020 17:18

Copy link

ContributorAuthor

roaffix commentedNov 13, 2020

Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:
if <var> is None: orif <var> is not None should be preferred overif <var>: per PEP8 guidline -https://www.python.org/dev/peps/pep-0008/#programming-recommendations
if <var> == 0 or <some number> - not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead ofif <var>: which seems like checking for None and may lead to confusion.

You are right. Some variables are considered to beNone by default, so it should be checked. Nice catch! I'll fix that.
In other cases, some of your checks, e.g.,AF_PATH inlibrary.py check ifAF_PATH is None, but it should fail ifAF_PATH == "". Simple conditions change toif AF_PATH can fix such problems when the default value or variable type is not known.

roaffix force-pushed thefix/flake8-refactoring branch from9b328f1 toad77595Compare

November 14, 2020 00:12

syurkevi reviewed

Nov 14, 2020

View reviewed changes

arrayfire/array.py OutdatedShow resolvedHide resolved

arrayfire/array.py

		location = Source.device
		else:
		location = Source.host
		location = Source.device if is_device else Source.host

Copy link

Contributor

syurkeviNov 14, 2020

Choose a reason for hiding this comment

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

revert

Copy link

ContributorAuthor

roaffixNov 14, 2020

Choose a reason for hiding this comment

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

IMO, it's better now :)

arrayfire/arith.py

		high_arr = constant_array(high, vdims[0], vdims[1], vdims[2], vdims[3], vty)
		else:
		high_arr = high.arr
		low_arr = low.arr if is_low_array else constant_array(low, vdims[0], vdims[1], vdims[2], vdims[3], vty)

Copy link

Contributor

syurkeviNov 14, 2020

Choose a reason for hiding this comment

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

revert

Copy link

ContributorAuthor

roaffixNov 14, 2020

Choose a reason for hiding this comment

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

Still, I don't see a point to revert these changes. These variables are just temp variables and they are not changed later anyhow. But we drop extra 7 lines of code.