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

Add extra check in fix_job_kwargs #2829

Merged
merged 4 commits into from
May 10, 2024
Merged

Conversation

zm711
Copy link
Collaborator

@zm711 zm711 commented May 10, 2024

For functions that take in an n_jobs argument they all run fix_job_kwargs, but if the user gives something like 4.0 to mean 4 jobs they end up with (if they have 4 cores)

int(4.0 * 4.0) = 16

So instead we should cast 4.0 -> 4 and just make it 4 jobs no?

What do you think @alejoe91 ?

@@ -87,9 +87,14 @@ def fix_job_kwargs(runtime_job_kwargs):
n_jobs = job_kwargs["n_jobs"]
assert isinstance(n_jobs, (float, np.integer, int))
if isinstance(n_jobs, float):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(n_jobs, float):
if isinstance(n_jobs, float) and n_jobs <= 1:

@zm711 maybe this is better?

Copy link
Member

Choose a reason for hiding this comment

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

we would need to add another:

else:
    n_jobs = int(n_jobs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So flip the if-else so the more common path is triggered first?

if isinstance(n_jobs, float) and n_jobs <= 1:
    int(n_jobs * os.cpu_counts())
else:
    int(n_jobs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see you're saying fuse all into if-elif-else!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know what you think of this!

Copy link
Member

Choose a reason for hiding this comment

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

Yep looks great!

@zm711
Copy link
Collaborator Author

zm711 commented May 10, 2024

I personally think that we should raise an error if someone sets 0 jobs rather than fix it for them....

Should I change that test or revert raising the error?

@alejoe91
Copy link
Member

alejoe91 commented May 10, 2024

I personally think that we should raise an error if someone sets 0 jobs rather than fix it for them....

Should I change that test or revert raising the error?

The old behavior was to set n_jobs=1 if n_jobs=0, right? I agree that raising an error is better.
Maybe one wants to type 10 and misses the 1 ;)

@alejoe91 alejoe91 added the core Changes to core module label May 10, 2024
@alejoe91 alejoe91 merged commit 707f78a into SpikeInterface:main May 10, 2024
11 checks passed
@zm711 zm711 deleted the n_jobs_check branch May 13, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants