add manual NumPy patching to mkl_fft

[ndgrigorian]

this PR proposes implementing manual patching for NumPy a la mkl_umath to mkl_fft

this patching is specifically designed with NEP-36 in mind

[Copilot]

Pull request overview

This PR introduces manual NumPy patching functionality to mkl_fft, following NEP-36 fair play guidelines. It allows users to dynamically replace NumPy's FFT functions with mkl_fft's optimized implementations at runtime, similar to the approach used in mkl_umath.

Changes:

• Added _patch_numpy.py module with patching/restoration functions and a context manager
• Added test file test_patch.py to verify basic patch/restore functionality
• Exported new public API functions: patch_numpy_fft(), restore_numpy_fft(), is_patched(), and mkl_fft context manager

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
mkl_fft/_patch_numpy.py	Implements the patching mechanism using thread-local storage to track state, provides functions to patch/restore numpy.fft functions, and includes a context manager for temporary patching
mkl_fft/tests/test_patch.py	Basic test verifying patch and restore functionality by checking module names
mkl_fft/init.py	Exports the new patching API functions and context manager class

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Using thread-local storage for tracking patch state creates a mismatch between local tracking and global effects. When one thread calls patch_numpy_fft(), it modifies the global np.fft namespace (affecting all threads), but only sets _is_patched=True in its own thread-local storage. This means: 1) Thread B won't see that Thread A has patched, and might try to patch again (triggering the double-patch issue), and 2) Thread A might restore while Thread B still needs the patch. Consider using a global lock and counter to properly coordinate multi-threaded patching, or document that this is not thread-safe for concurrent patch/restore calls.

[Copilot]

The new patching functionality lacks documentation. Consider adding: 1) docstrings to all public functions (patch_numpy_fft, restore_numpy_fft, is_patched), 2) docstring to the context manager class explaining its usage, 3) updating README.md to explain this new feature and provide usage examples, and 4) adding a note about thread-safety considerations.

[Copilot]

The class name "mkl_fft" conflicts with the module name "mkl_fft". This creates ambiguity: when users import mkl_fft, they get the module, but mkl_fft.init.py also exports a class called mkl_fft. This makes it impossible to use the context manager class directly without confusion. Consider renaming the class to something like "patch_context" or "numpy_fft_context" to avoid this naming collision.

[Copilot]

Calling do_patch() multiple times without calling restore() in between will overwrite _restore_dict values, losing the original NumPy functions. This prevents proper restoration. Add a check: if self._is_patched is True, either skip re-patching or raise an error to prevent this issue.

Suggested change

	def do_patch(self):
	def do_patch(self):
	# Avoid re-patching when already patched, which would overwrite
	# the originals stored in _restore_dict with the patched versions.
	if self._is_patched:
	return

[Copilot]

The restore method iterates over _restore_dict.keys() instead of the original patched_functions list. This means if do_patch() was interrupted partway through (e.g., due to an exception), only the functions that were successfully patched will be restored. While this might be the intended behavior, it could leave numpy in an inconsistent state. Consider whether this is the desired behavior or if all functions should be attempted for restoration.

Suggested change

	for name in self._restore_dict.keys():
	# Attempt to restore all functions that are part of the patching contract.
	# _restore_func will safely handle cases where no original value was recorded.
	for name in self.__patched_functions__:

[Copilot]

The condition check is redundant. The expression "(getattr(_tls, 'initialized', None) is not None) and (_tls.initialized is True)" can be simplified to just "getattr(_tls, 'initialized', False)" since the attribute is always set to True when initialized. The current check is unnecessarily complex.

Suggested change

	return (getattr(_tls, "initialized", None) is not None) and (
	_tls.initialized is True
	)
	return getattr(_tls, "initialized", False)

[Copilot]

Using class attributes instead of instance attributes is a code smell. While each thread gets its own _Patch instance via thread-local storage, using class attributes like _is_patched and _restore_dict suggests they're shared across instances, which is misleading. These should be initialized in an init method as instance attributes (self._is_patched = False, self._restore_dict = {}, self.patched_functions = _nfft.all) to make the code clearer and avoid potential issues if the class is ever used outside of TLS.

Suggested change

	_is_patched = False
	__patched_functions__ = _nfft.__all__
	_restore_dict = {}
	def __init__(self):
	# Per-instance state for patching; one _Patch instance is stored per-thread.
	self._is_patched = False
	self.__patched_functions__ = _nfft.__all__
	self._restore_dict = {}

[Copilot]

After restoring all functions, the _restore_dict should be cleared to prevent memory leaks and allow for proper re-patching. Add "self._restore_dict.clear()" after the loop on line 69.

Suggested change

	self._restore_func(name, verbose=verbose)
	self._restore_func(name, verbose=verbose)
	self._restore_dict.clear()

[Copilot]

The test coverage is incomplete. Consider adding tests for: 1) the context manager usage (with mkl_fft.mkl_fft() or similar), 2) testing that patched functions produce correct results, 3) edge cases like double-patching without restoring, 4) thread safety if TLS is being used, and 5) verbose output.