Skip to content

Conversation

androids-electric-sheep
Copy link
Contributor

Based on #1539

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.09%. Comparing base (6310841) to head (44f89ca).

❗ Current head 44f89ca differs from pull request most recent head f6f607f. Consider uploading reports for the commit f6f607f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1549      +/-   ##
==========================================
- Coverage   95.16%   95.09%   -0.07%     
==========================================
  Files          44       44              
  Lines        7562     7567       +5     
==========================================
  Hits         7196     7196              
- Misses        366      371       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fcharras
Copy link
Contributor

Thanks for the PR @androids-electric-sheep, can you give a bit more context about what is the current behavior, does it fail if it's not an int ? (what about n_jobs=3.0, n_jobs="3"). So we know if the PR is about generating a better, earlier error message, or actually add constraints on the input type that didn't exist before.

@androids-electric-sheep
Copy link
Contributor Author

From the issue this PR is based on, and reproduced locally using the example code provided, if a float is provided to the n_jobs parameter slicing will later fail because it requires an int.

So it's a combination of both really, adding the type constraint and then providing a more understandable error message if it's violated

@tomMoral
Copy link
Contributor

tomMoral commented Apr 3, 2024

Hello! Thanks for the PR!
We decided to go with a more permissive behavior (try to convert to int else fail).

We are trying to make a release by Friday so I changed the PR directly and made the test cover more cases.
Thanks for the contrib!

@tomMoral
Copy link
Contributor

tomMoral commented Apr 3, 2024

The failure in the CI is unrelated, merging this PR.

@tomMoral tomMoral merged commit ccfb803 into joblib:main Apr 3, 2024
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