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

Allow setting n_jobs on tool invocation #2430

Merged
merged 11 commits into from
Nov 23, 2023
Merged

Allow setting n_jobs on tool invocation #2430

merged 11 commits into from
Nov 23, 2023

Conversation

LukasNickel
Copy link
Member

Tobychev
Tobychev previously approved these changes Oct 26, 2023
Tobychev
Tobychev previously approved these changes Oct 26, 2023
ctapipe/reco/sklearn.py Outdated Show resolved Hide resolved
@maxnoe
Copy link
Member

maxnoe commented Oct 30, 2023

Maybe it also makes sense to set n_jobs=None for the models after training before pickling? So that the pickled models have n_jobs=None?

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2e1011b) 92.47% compared to head (0372d11) 92.46%.

Files Patch % Lines
ctapipe/tools/apply_models.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2430      +/-   ##
==========================================
- Coverage   92.47%   92.46%   -0.02%     
==========================================
  Files         234      234              
  Lines       19819    19868      +49     
==========================================
+ Hits        18327    18370      +43     
- Misses       1492     1498       +6     

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

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Please update the config files so that they now pass n_jobs to the Reconstructor classes and not in the model configs.

Tobychev
Tobychev previously approved these changes Nov 17, 2023
- It can be useful to set n_jobs indepently from the config file
- For the sklearn reconstructors this requires setting n_jobs on every
  model, that is attached to the reconstructor
- Fixes #2307
- Implement the @observe method there as well, because it is not a
subclass of sklearnreconstructor
- Move n_jobs from model config to reconstructor config
@maxnoe maxnoe merged commit 4a45dcd into main Nov 23, 2023
12 of 14 checks passed
@maxnoe maxnoe deleted the n_jobs branch November 23, 2023 17:26
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.

Allow overriding number of cores used by reconstructors on the CLI
4 participants