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 optional n_processes kw to backtest_many #179

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

Conversation

quant5
Copy link
Contributor

@quant5 quant5 commented Sep 3, 2024

Solution to #177

Tested on my machine running Windows and Python 3.11.5.
Original behavior (which errors for me, hence #177) is retained if parameter is not supplied.

@enzbus It's up to you whether to keep multiprocess. (currently, it's installed as a dependency.)

I hope the formatting changes are ok - my formatter (black) automatically does those.

@enzbus
Copy link
Collaborator

enzbus commented Sep 3, 2024

Looks like there's a ton of changes because your editor ran black? I was thinking to set number or processes to something like min(cpu_count(), n) where n is the number of policies passed to backtest_many. Let me have a closer look though. Good that you found the fix to #177 anyways.

@quant5
Copy link
Contributor Author

quant5 commented Sep 3, 2024

yeah, the only actual change is here https://github.com/cvxgrp/cvxportfolio/pull/179/files#diff-ceed8a0828d51fb92ef677933274ae79b2d6f6f8082281b8fb449b6aae636892R843

your approach of min() is fine by me as well.

@enzbus enzbus changed the base branch from enzbus-patch-1 to master September 4, 2024 14:50
enzbus added a commit that referenced this pull request Sep 4, 2024
now it creates min(len(policies), cpu_count) processes. This fixes #177 and replaces
#179. In general it does not hurt, previous behavior was to always create cpu_count()

processes, some were probably unused.
@enzbus
Copy link
Collaborator

enzbus commented Sep 4, 2024

I just merged in master the code that uses min(cpu_count(), len(policies)), I think it's good. Not sure though, leaving this PR open since it may be useful to provide num_processes explicitely.

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.

2 participants