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

Initialize empty lists with None instead of object instances #338

Open
alexbarcelo opened this issue Mar 5, 2021 · 1 comment · May be fixed by #416
Open

Initialize empty lists with None instead of object instances #338

alexbarcelo opened this issue Mar 5, 2021 · 1 comment · May be fixed by #416

Comments

@alexbarcelo
Copy link
Contributor

This is a minor issue that should have no relevant impact in performance (and, for certain, not a negative one). But, IMHO, it is a boost in readability and friendlier to typical Python idioms.

There are a total of 26 places where a list is initialized with the following pattern:

    some_blocks = [object() for _ in range(number_of_blocks)]

AFAICT, there is no need to initialize object instances in order to populate the list. This is a burden for readability and for the garbage collector. I have not investigated the 26 different usages of that anti-idiom, but in the ones I have looked at, the instantiated object is never used and is simply a placeholder.

I believe that the following is much more readable for Python standards and avoids flooding the GC:

    some_blocks = [None] * number_of_blocks

Because None is a singleton object (in Python), the suggested code will be lighter. I expect that to be "the chocolate of the parrot" regarding performance, but again, "Simple is better than complex & Readability counts".

@cTatu cTatu added this to the release 0.8 milestone Oct 10, 2022
@cTatu cTatu linked a pull request Nov 8, 2022 that will close this issue
3 tasks
@cTatu
Copy link
Collaborator

cTatu commented Nov 9, 2022

Check if can use:

dislib/dislib/data/array.py

Lines 1312 to 1315 in 7fdd24d

return Array(blocks=[[None] for _ in range(math.ceil(shape[0] /
block_size[0]))],
top_left_shape=block_size,
reg_shape=block_size, shape=shape, sparse=False)

@cTatu cTatu linked a pull request Nov 9, 2022 that will close this issue
3 tasks
@cTatu cTatu removed this from the release 0.8 milestone Nov 10, 2022
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 a pull request may close this issue.

2 participants