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

Update defaults.ini #2035

Merged
merged 7 commits into from
Jul 7, 2022
Merged

Update defaults.ini #2035

merged 7 commits into from
Jul 7, 2022

Conversation

DidierRLopes
Copy link
Collaborator

Fixes #2024

FYI @deeleeramone

@DidierRLopes DidierRLopes added the bug Fix bug label Jul 3, 2022
@JerBouma
Copy link
Contributor

JerBouma commented Jul 6, 2022

Almost:

2022 Jul 06, 08:55 (🦋) /portfolio/po/params/ $ file defaults.ini
Parameters:
    historic_period    : 3y
    log_returns        : 0
    return_frequency   : d
    max_nan            : 0.05
    threshold_value    : 0.3
    nan_fill_method    : time
    significance_level : 0.05


2022 Jul 06, 08:55 (🦋) /portfolio/po/params/ $ arg significance_level 0.10
Error: option values must be strings

2022 Jul 06, 08:55 (🦋) /portfolio/po/params/ $ arg significance_level '0.10'
The value '0.10' is not an option for significance_level.
The value needs to be between 0.0 and 1.0 in steps of 0.001

2022 Jul 06, 08:55 (🦋) /portfolio/po/params/ $ arg significance_level 0.10
Error: option values must be strings

@DidierRLopes
Copy link
Collaborator Author

@JerBouma that is not a regression, seems the issue was already there.

Screenshot 2022-07-06 at 13 59 16

We should have tests around this to catch these issues, seems the problem is on the setting up of these values.

@JerBouma
Copy link
Contributor

JerBouma commented Jul 6, 2022

Agreed but I figured we should be fixing that too while we are at it. I approve of the changes you made so far.

@DidierRLopes
Copy link
Collaborator Author

@JerBouma can you please have a go at this? Very constrained time-wise, hopefully all works well

@JerBouma
Copy link
Contributor

JerBouma commented Jul 7, 2022

@DidierRLopes The reason we didn't catch this because it works perfectly fine for the Excel version.

2022 Jul 07, 03:56 (🦋) /portfolio/po/params/ $ file OpenBB_Parameters_Template_v1.0.0.xlsx
Parameters:
    historic_period         : 3y
    log_returns             : 0
    return_frequency        : d
    max_nan                 : 0.05
    threshold_value         : 0.3
    nan_fill_method         : time
    risk_free               : 0.003
    significance_level      : 0.05
    risk_measure            : MV
    target_return           : -1
    target_risk             : -1
    expected_return         : hist
    covariance              : hist
    smoothing_factor_ewma   : 0.94
    long_allocation         : 1
    short_allocation        : 0


2022 Jul 07, 03:56 (🦋) /portfolio/po/params/ $ arg max_nan 0.10

2022 Jul 07, 03:56 (🦋) /portfolio/po/params/ $ ?
╭────────────────────────────────────────────────────────────────── Portfolio - Portfolio Optimization - Parameters ───────────────────────────────────────────────────────────────────╮
│                                                                                                                                                                                      │
│ Loaded file: OpenBB_Parameters_Template_v1.0.0.xlsx                                                                                                                                  │
│                                                                                                                                                                                      │
│     file               load portfolio risk parameters                                                                                                                                │
│     save               save portfolio risk parameters to specified file                                                                                                              │
│                                                                                                                                                                                      │
│ Model of interest: maxsharpe                                                                                                                                                         │
│                                                                                                                                                                                      │
│     clear              clear model of interest from filtered parameters                                                                                                              │
│     set                set model of interest to filter parameters                                                                                                                    │
│     arg                set a different value for an argument                                                                                                                         │
│                                                                                                                                                                                      │
│ Parameters:                                                                                                                                                                          │
│     historic_period         : 3y                                                                                                                                                     │
│     log_returns             : 0                                                                                                                                                      │
│     return_frequency        : d                                                                                                                                                      │
│     max_nan                 : 0.1                                                                                                                                                    │
│     threshold_value         : 0.3                                                                                                                                                    │
│     nan_fill_method         : time                                                                                                                                                   │
│     risk_free               : 0.003                                                                                                                                                  │
│     significance_level      : 0.05                                                                                                                                                   │
│     risk_measure            : MV                                                                                                                                                     │
│     target_return           : -1                                                                                                                                                     │
│     target_risk             : -1                                                                                                                                                     │
│     expected_return         : hist                                                                                                                                                   │
│     covariance              : hist                                                                                                                                                   │
│     smoothing_factor_ewma   : 0.94                                                                                                                                                   │
│     long_allocation         : 1                                                                                                                                                      │
│     short_allocation        : 0                                                                                                                                                      │
│                                                                                                                                                                                      │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── OpenBB Terminal v1.5.0 (https://openbb.co) ─╯

I fixed this by asking the user to change the file directly. We could also build in a bunch of logic where it converts from string to value every time but I feel like this works better.

2022 Jul 07, 04:03 (🦋) /portfolio/po/params/ $ file defaults.ini
Parameters:
    historic_period    : 3y
    log_returns        : 0
    return_frequency   : d
    max_nan            : 0.05
    threshold_value    : 0.3
    nan_fill_method    : time
    significance_level : 0.05


2022 Jul 07, 04:03 (🦋) /portfolio/po/params/ $ arg max_nan 0.10
Please adjust the parameters directly in the defaults.ini file.

2022 Jul 07, 04:03 (🦋) /portfolio/po/params/ $

@JerBouma JerBouma merged commit 0276784 into main Jul 7, 2022
@JerBouma JerBouma deleted the 2024 branch July 7, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] /portfolio/po/params - default.ini contains invalid parameters - i.e. 5% instead of 0.05
2 participants