Skip to content

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