Skip to content

Docs: Edit the argument documentation of n_jobs of the parallelize function#987

Merged
selmanozleyen merged 4 commits intoscverse:mainfrom
selmanozleyen:docs/spatial-autocorr
May 15, 2025
Merged

Docs: Edit the argument documentation of n_jobs of the parallelize function#987
selmanozleyen merged 4 commits intoscverse:mainfrom
selmanozleyen:docs/spatial-autocorr

Conversation

@selmanozleyen
Copy link
Copy Markdown
Member

@selmanozleyen selmanozleyen commented Apr 9, 2025

Related: #957

Description

Following the discussion in #984 (comment) I modified the docs. I also wrote that this argument will be removed in the future based on our conversations but I am not sure about that.

If that is correct I will create an issue titled: Reconsidering usage of paralellize or something like that

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.58%. Comparing base (3771a0a) to head (7991dbc).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #987   +/-   ##
=======================================
  Coverage   66.58%   66.58%           
=======================================
  Files          40       40           
  Lines        6057     6057           
  Branches     1014     1014           
=======================================
  Hits         4033     4033           
  Misses       1663     1663           
  Partials      361      361           
Files with missing lines Coverage Δ
src/squidpy/_docs.py 94.20% <ø> (ø)
src/squidpy/_utils.py 57.43% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ilan-gold
Copy link
Copy Markdown
Contributor

  1. I would add a FutureWarning about this behavior
  2. That sounds reasonable to make a folllow-up issue. Let's try to lay out everywhere this sort of thing occurs and then we can proceed with which ones can be removed (and given FutureWarnings)
  3. No need to ping, a review is fine :)

@selmanozleyen
Copy link
Copy Markdown
Member Author

@ilan-gold I could send a future warning but are we going to deprecate something else if we are going to replace parallelize? I feel like we should throw warnings if we will surely replace it because many functions seem to use parallelize and we might be sending annoying warnings for a long time. Maybe I can also just remove the sentence where I say n_jobs will be removed in the future and just mention that it us to numba ultimately how many cores will be utilized.

@selmanozleyen selmanozleyen requested a review from ilan-gold May 8, 2025 15:43
Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

I would say this looks ok given the uncertainty, which can hopefully be resolved, and with the caveat that I don't fully understand why we're using two different parallelization frameworks (numba + joblib or whatever parallelize uses).

@selmanozleyen
Copy link
Copy Markdown
Member Author

@ilan-gold I also don't understand, I think @Intron7 had an opinion on the parallelize utility function. If it's worth refactoring maybe we should consider but maybe we can merge this if thats still fine

@selmanozleyen selmanozleyen merged commit e44440e into scverse:main May 15, 2025
7 checks passed
@flying-sheep flying-sheep linked an issue May 15, 2025 that may be closed by this pull request
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.

3 participants