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

Bugfix - Use Numpy array instead of mmap #11

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

omer-dayan
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@noa-neria noa-neria left a comment

Choose a reason for hiding this comment

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

The only thing is that we add dependency on numpy, for creating a continuous buffer.
If we ever find another solution, it'd be better to remove it.

@@ -1,5 +1,6 @@
import os
from typing import List, Iterator
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we import just the empty. i.e. from numpy import empty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In python, importing partial package does not reduce import time (https://stackoverflow.com/questions/710551/use-import-module-or-from-module-import).

Its mainly convention.

I prefer keeping import numpy as np because it keep us with the context.
I mean comparing np.empty(... vs empty(...
In my opinion its more convenient to see the first as you immediately understand its numpy array.
In addition to that, it became a convention in the community to import numpy as np

For example, can you imagine people are doing from torch import zeros and using zeros(... instead of torch.zeros(... in the middle of the code?

@omer-dayan omer-dayan merged commit ee5d3d0 into master Oct 28, 2024
1 check passed
@omer-dayan omer-dayan deleted the omer/bugfix-dealloc-buffer branch October 28, 2024 14:28
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.

2 participants