Skip to content

fix issues with set_workers #183

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix issues with set_workers #183

wants to merge 1 commit into from

Conversation

vtavana
Copy link
Collaborator

@vtavana vtavana commented May 15, 2025

resolves #177

@vtavana vtavana self-assigned this May 15, 2025
@vtavana vtavana force-pushed the fix-set-workers branch 4 times, most recently from df1bbd8 to f5d3eb0 Compare May 16, 2025 17:48
@vtavana vtavana force-pushed the fix-set-workers branch from f5d3eb0 to 8fe0f1e Compare May 16, 2025 17:53
@vtavana vtavana marked this pull request as ready for review May 16, 2025 18:22
@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 18:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issues with the set_workers functionality in the SciPy interface of mkl_fft, aligning the default workers value with the maximum number of threads provided by MKL instead of the CPU count. Key changes include updating tests to use mkl.get_max_threads(), refactoring the logic in _workers_to_num_threads, and updating documentation and changelog accordingly.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
mkl_fft/tests/third_party/scipy/test_multithreading.py Updated tests to use mkl.get_max_threads() instead of os.cpu_count() and adjusted expected default values.
mkl_fft/interfaces/_scipy_fft.py Refactored workers handling and error messaging to rely on MKL thread counts.
mkl_fft/interfaces/README.md Updated documentation to reflect the new default behavior for workers.
CHANGELOG.md Documented the fixes to the set_workers functionality.
Comments suppressed due to low confidence (2)

mkl_fft/tests/third_party/scipy/test_multithreading.py:56

  • [nitpick] The slow test marker is currently commented out. Confirm that disabling this test is intentional, and if not, consider re-enabling it with proper configuration to ensure full test coverage.
# @pytest.mark.slow

mkl_fft/interfaces/_scipy_fft.py:103

  • [nitpick] Replacing os.cpu_count() with mkl.get_max_threads() ensures consistency with the default worker behavior, but verify that this approach aligns with expected performance and behavior on systems where the two values might differ.
    # SciPy uses os.cpu_count()

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.

set_workers from SciPy interface does not work as expected
1 participant