I am currently finalising the class for a trading strategy and wanted to know if the below could be re-written in a shorter way that will let it run faster?

This is PCA1 df:

                         USDJPY       EURUSD     GBPUSD      AUDUSD      GBPAUD
date    pca_component                   
2019-10-08  weights_2   0.438064    -0.546646   0.433956    0.411839    0.389035
2019-10-09  weights_2   -0.976341   -0.002841   -0.089620   -0.010523   0.196488
2019-10-10  weights_2   -0.971039   -0.009815   0.214701    0.048983    -0.092149
2019-10-11  weights_2   -0.747259   0.104055    -0.174377   -0.287954   0.563429
2019-10-14  weights_2   0.026438    -0.040705   0.715500    -0.200801   -0.667370
... ... ... ... ... ... ...
2021-09-29  weights_2   0.882239    -0.018798   0.355097    0.279886    -0.129888
2021-09-30  weights_2   0.328128    -0.901789   -0.062934   -0.061846   0.267064
2021-10-01  weights_2   0.954694    0.020833    -0.111151   0.250181    -0.114808
2021-10-04  weights_2   -0.653506   -0.490579   -0.296858   -0.483868   -0.100047
2021-10-05  weights_2   0.221695    0.738876    0.156838    0.322064    0.525919

Backtesting function - just for background

import bt # backtesting library
# Backtesting package
def backtesting(self, signal, price, commission, initial_capital=10000, strategy_name='Strategy', progress_bar=False):
# Define the strategy
bt_strategy = bt.Strategy(strategy_name, 
def my_comm(q, p):
    return commission

bt_backtest = bt.Backtest(bt_strategy, price, initial_capital=initial_capital,  commissions=my_comm, progress_bar=progress_bar, integer_positions=True)
bt_result = bt.run(bt_backtest)
return bt_result, bt_backtest

Now, the code:

store_nlargest = []
for i in range(5, 10):
    n_largest_weights = PCA1.nlargest(i, returns.columns)

store_df = []
results = []
for i in range(len(store_nlargest)):    
    create_df = pd.DataFrame(store_nlargest [i]) 
    labels = [f"Part_{u+1}" for u in range(len(store_nlargest))]
    store_df .append(create_df)
    df_concat = pd.concat(store_df, keys=labels)
    weights = df_concat[df_concat.index.get_level_values(0)==labels[i]]
    weights = weights.droplevel(1)
    backtest_returns = backtesting(weights, prices, 3, initial_capital=10000000, strategy_name='Strategy', progress_bar=True)
    results.append(backtest_returns )
  • 1
    \$\begingroup\$ Please revisit your indentation - it's not going to work in its current state \$\endgroup\$
    – Reinderien
    Oct 6 at 19:14
  • 1
    \$\begingroup\$ I'm new to codereview so I don't know if this should be answer or comment. But if you want better performance, you don't need to call .nlargest multiple times. Just call it with the largest value (9) and select the first i rows. And you don't need to create an intermediary dataframe and put them in lists. It would become less readable but generator expressions would probably be faster. Then again I really don't understand why you're concatenating dataframes and selecting the one you created in that loop? The values of create_df and weights are identical? \$\endgroup\$
    – kubatucka
    Oct 6 at 21:44
  • \$\begingroup\$ Yeah, I made errors in my original code. I have made amends which is above though I will try your largest value suggestion. \$\endgroup\$
    – Kale
    Oct 7 at 2:31
  • 2
    \$\begingroup\$ I rolled back the last edit. It is against this review policy to update the code after the answer has been posted., because it invalidates the answer. \$\endgroup\$
    – vnp
    Oct 7 at 2:52
  • 1
    \$\begingroup\$ @vpn; Ok, but it sounds like the original code was not correct. Vote to close? (Sorry @Kale!) \$\endgroup\$ Oct 7 at 18:17

The structure of your code is weird; are you sure it's doing what you want? If it is, strongly consider adding comments or better variable names or something to make it clearer what it's supposed to be doing. Without such explanation, it looks like you have code inside your loop that ought to go outside it.

That said, I think this is the same behavior as what you've written:

store_nlargest = [PCA1.nlargest(i, returns.columns)
                  for i
                  in range(5, 10)]
labels = [f"Part_{u+1}" for (u, _) in enumerate(store_nlargest)]

store_df = []  # persistent? reused? 

def make_weights(i, nlargest):
    store_df.append(pd.DataFrame(nlargest))  # still contains data from prior call?
    df_concat = pd.concat(store_df, keys=labels)  # needs better name.
    return df_concat[
        df_concat.index.get_level_values(0) == labels[i]

results = [backtesting(make_weights(i, nlargest), prices, 3,
           for (i, nlargest)
           in enumerate(store_nlargest)]

This isn't perfect, but I think it does a better job of making its weirdness explicit. You could make a case that the results list-comprehension should still be a for loop because it contains mutation of store_df as a side-effect; I don't really understand what supposed to be happening there so I won't take a side.

A lot of folks in python like to use lists for everything, but they're not perfect for most things. They're mutable, and it's nice to have everything that can be immutable be immutable. They're also "eager", whereas lazy generators aren't much more work to write (but do require careful use).

  • \$\begingroup\$ You are right the code does not do what I wanted it to do - I made blunders. I have updated it, though the issue is speed is more speed than length. I have updated the code which does what I need it to do. It creates weights of the assets based on the nlargest n range and then runs the backtest. Is there a way to speed it up? \$\endgroup\$
    – Kale
    Oct 7 at 2:28
  • \$\begingroup\$ Is there an nlargest outside make_weights(i, nlargest)? \$\endgroup\$
    – greybeard
    Oct 11 at 2:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.