Skip to content

Conversation

@SaurabhJamadagni
Copy link
Contributor

The PR is in reference to issue:

PR makes the following changes:

  • Creates a Portfolio struct which holds:
    • tickers
    • mean returns vector for assets
    • covariance_matrix
    • risk-free rate for the market to use
    • expected return for the portfolio
    • method of calculating returns (log or simple)
    • weights for the assets
    • returns calculated (internal)
  • In it's current state, the struct takes a path to a .csv file which contains price data.
  • fn new() will read from the data file and perform return calculations and produce a covariance matrix.
  • ndarray and 'ndarray_stats` are used to store records from the csv and perform operations.

Example output:
image

Next steps:

  • Perform portfolio optimization using legrange multipliers and optimization crates

@carlobortolan please let me know your feedback on this. I haven't written tests for this yet. Was planning to include them when I add the optimization part. Can add something before as well if you would like that before merging. Let me know :)

@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 0% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.70%. Comparing base (6245aa0) to head (b991e58).

Files with missing lines Patch % Lines
src/portfolio/mean_variance.rs 0.00% 91 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   92.64%   89.70%   -2.95%     
==========================================
  Files          29       30       +1     
  Lines        2775     2866      +91     
==========================================
  Hits         2571     2571              
- Misses        204      295      +91     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SaurabhJamadagni
Copy link
Contributor Author

In the changes following this, I would also like to separate out the functions that calculate returns as they aren't really struct functions. Could move them to a more general utilities module or something that the whole package can make use of.

@carlobortolan
Copy link
Owner

Thanks for putting this together @SaurabhJamadagni; already looks really solid!
Just FYI: I'll be a bit busy with travelling & moving for the next 2.5 weeks (after that I'll also finally have time to regularly contribute to quantrs again and review/test new PRs thoroughly)

Two quick notes:

  • expect is used for csv reading/parsing; returning a Result might handle errors more gracefully

  • As you suggested, the return calculation functions could be moved to a utils module since they aren't strictly tied to the struct

Appreciate all the work so far!

P.S. Just merged #65 🚀

@SaurabhJamadagni
Copy link
Contributor Author

No worries on the delay @carlobortolan. Hope you have a stress free move!

expect is used for csv reading/parsing; returning a Result might handle errors more gracefully

Noted. I'll give it a look.

P.S. Just merged #65 🚀

Thanks on this one! Appreciate you pushing the final few commits to get it merged. Do we wanna keep this PR open till the whole optimization is implemented or are you looking to merge it after some of the above changes? No rush of course, I just wanted to clarify where you stand on this.

@carlobortolan
Copy link
Owner

Hey @SaurabhJamadagni

Sorry for the long delay - finally finished moving (and all the bureaucracy that comes with it 🙄), so I'll have some time to review it over the weekend.

Do we wanna keep this PR open till the whole optimization is implemented or are you looking to merge it after some of the above changes?

This PR is already quite well-sized with ~300 added LOC, so it's fine to leave it as is. (However, I'd say that it would be better to have the whole optimization merged as one PR, so that master doesn't contain too many placeholders / TODOs, but I'll let you know once I've tested this PR, as this might depend on how it's implemented.)

@SaurabhJamadagni
Copy link
Contributor Author

Sorry for the long delay

No worries @carlobortolan! I know the headache that comes with moving. I hope everything went well.

However, I'd say that it would be better to have the whole optimization merged as one PR, so that master doesn't contain too many placeholders / TODOs

I agree with this and hence was curious. I also wanted to check if you would consider having dev and release branch instead of merging with main. Incomplete features could stay on dev and after a certain amount of features are added or after a certain period of time you could merge dev into main as a release which could be documented through the current CHANGELOG or something. Do you think there's a benefit to such a separation?

@carlobortolan
Copy link
Owner

carlobortolan commented Oct 15, 2025

I also wanted to check if you would consider having dev and release branch instead of merging with main. Incomplete features could stay on dev and after a certain amount of features are added or after a certain period of time you could merge dev into main as a release which could be documented through the current CHANGELOG or something. Do you think there's a benefit to such a separation?

@SaurabhJamadagni: Yes, having a dev branch could make things more organized, especially for tracking unreleased features. That said, I think that setup usually benefits larger projects more. If we had master, dev and feature branches, it might introduce redundancy since I’d need to review code before merging into dev, and then again when merging dev into master 😅

However, what we could do is keep master as the release branch and handle ongoing work through feature branches. For example, in this case we could have portfolio_optimization_structs and portfolio_optimization_mean_variance as sub-branches merged into the main feature branch portfolio_optimization.

This way, features that are less modular / need multiple PRs can be split into smaller branches& PRs, while others can still be merged in a single PR. Does this approach sound good?

(P.S.: I took the liberty of fixing the merge conflicts here, since I just updated some dependencies in another PR which might cause some issues with our Cargo.lock.MSRV otherwise)

@carlobortolan carlobortolan self-assigned this Oct 15, 2025
@carlobortolan carlobortolan added enhancement New feature or request dependencies Pull requests that update a dependency file rust Pull requests that update rust code labels Oct 15, 2025
Copy link
Owner

@carlobortolan carlobortolan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and again sorry for the long review-time: The PR looks mostly good to me; just added a few very small comments 👍

edit: another thing I noticed: The weights field is Option<Vec<f64>> but currently never populated. Is this intended? It might be a good idea to add a method stub/placeholder for computing optimal weights, so we don't forget to implement this later.


#[warn(unused_variables)]
fn main() {
let data_path = "/Users/moneymaker/Downloads/ETFprices.csv";
Copy link
Owner

Choose a reason for hiding this comment

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

Use std::env::args() or a relative path to an example file (e.g., examples/data/ETFprices.csv) to avoid hard coded paths.

fn calculate_simple_returns(prices: &Array2<f64>) -> Array2<f64> {
let simple_returns =
(&prices.slice(s![1.., ..]) - &prices.slice(s![..-1, ..])) / prices.slice(s![..-1, ..]);
simple_returns.to_owned()
Copy link
Owner

@carlobortolan carlobortolan Oct 15, 2025

Choose a reason for hiding this comment

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

calculate_simple_returns and calculate_log_returns currently create new arrays using .to_owned(). Consider in-place operations or preallocating arrays for large datasets.

rand_distr = "0.5.1"
rayon = "1.10.0"
statrs = "0.18.0"
ndarray = "0.16.1"
Copy link
Owner

Choose a reason for hiding this comment

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

I think the latest version is 0.17.0 - is there any reason for using 0.16.x? If not, we should probably upgrade to the newer version

fn calculate_log_returns(prices: &Array2<f64>) -> Array2<f64> {
let log_prices = prices.mapv(|x| x.ln());
let log_returns = &log_prices.slice(s![1.., ..]) - &log_prices.slice(s![..-1, ..]);
log_returns.to_owned()
Copy link
Owner

@carlobortolan carlobortolan Oct 15, 2025

Choose a reason for hiding this comment

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

see comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants