-
Notifications
You must be signed in to change notification settings - Fork 532
[REF] removed all uses of numpy_mmap #3121
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
[REF] removed all uses of numpy_mmap #3121
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3121 +/- ##
=========================================
- Coverage 67.83% 65% -2.83%
=========================================
Files 295 295
Lines 39302 39290 -12
Branches 5178 5178
=========================================
- Hits 26661 25541 -1120
- Misses 11935 12699 +764
- Partials 706 1050 +344
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. One potential problem is removing the NUMPY_MMAP
constant altogether, if any downstream projects use it. Perhaps we should leave it (anybody have strong opinions?) for now, though we can simply make it True
.
If we are going to leave it, we should have a plan for removing it, including how to notify developers that it's going away. Raising a DeprecationWarning
would require some tricky hacks, and might not be worth it. Possibly just a warning in the release notes?
WDYT?
@@ -28,7 +28,6 @@ | |||
"filemanip_level": ("logging.utils_level", "1.0"), | |||
} | |||
|
|||
NUMPY_MMAP = LooseVersion(np.__version__) >= LooseVersion("1.12.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves the numpy
import unused.
i think we should just remove it. while not explicit, this was not intended to be an external api. hopefully developers will catch this via release candidates if they were re-using this constant. |
Works for me. |
Summary
Removed all use cases of numpy_mmap as it will always be true.
Acknowledgment
I acknowledge that this contribution will be available under the Apache 2 license.