FactorModelSigma -> FactorModelCovariance

[wec7]

Hi @enzbus,

I noticed the implementation of FactorModelSigma (renamed as FactorModelCovariance) has changed from F D F^T to F F^T where F is factor loadings, and D was the factor sigma (or factor covariance).

Just curious any reasons? I can try implement and send PR to add factor_Sigma back, but prior to that, may I make sure if any bottlenecks you've experienced?

Old implementation -

cvxportfolio/cvxportfolio/risks.py

Lines 157 to 162 in 1786671

	def _estimate(self, t, wplus, z, value):
	self.expression = cvx.sum_squares(cvx.multiply(
	np.sqrt(values_in_time(self.idiosync, t)), wplus)) + \
	cvx.quad_form((wplus.T * values_in_time(self.exposures, t).values.T).T,
	values_in_time(self.factor_Sigma, t).values)
	return self.expression

New implementation:

cvxportfolio/cvxportfolio/risks.py

Lines 295 to 304 in a94f9c1

	def _compile_to_cvxpy(self, w_plus, z, w_plus_minus_w_bm):
	self.expression = cp.sum_squares(cp.multiply(
	self.idyosync_sqrt_parameter, w_plus_minus_w_bm[:-1]))
	assert self.expression.is_dcp(dpp=True)
	self.expression += cp.sum_squares(self.F_parameter @
	w_plus_minus_w_bm[:-1])
	assert self.expression.is_dcp(dpp=True)
	return self.expression

Thanks,

[enzbus]

Hi, you are right thanks for your comment. It's just a matter of simplification, you can always take a square root decomposition (like np.linalg.cholesky) of the factor Sigma and matrix multiply it by F to fit in this new framework (that would be done internally if I implement the change you suggest). I can add it as an optional argument to the constructor, maybe it's easier.

It's also a bit more difficult (not a one liner change) because of the new implementation with cvxpy parameters, it restricts the allowed syntax. Let me think about it, I'll make a fix.

[enzbus]

Hello, this has been added with commit 7827bc6 , I'll make a release probably today. I also clarified the various combination of arguments in the docstring.

[wec7]

Thank you very much