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

Add argument to remove masking for heatmap in hcorr and print correlations to command line #1913

Merged
merged 20 commits into from
Jun 8, 2022

Conversation

stkerr
Copy link
Contributor

@stkerr stkerr commented Jun 7, 2022

Description

This PR adds an argument to the hcorr command to toggle displaying either the masked correlation matrix (as it does today) or a full, unmasked correlation matrix. The behavior defaults to off, which means the command will behave as it does today without the flga.

This PR also prints the correlation matrix to the command line. This is helpful if someone is using the terminal over SSH or otherwise doesn't want to view the results only graphically. The print logic will wrap large matrices across multiple lines, rather than truncating the column.

This is after the change and using the flag:
image

How has this been tested?

Several tests were run with different tickers.
Pytest was run and all tests passed.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@stkerr stkerr force-pushed the stkerr-remove-heatmap-masking branch from ea32c2f to b134019 Compare June 7, 2022 15:38
@stkerr stkerr changed the title Remove masking for heatmap Remove masking for heatmap in hcorr and print correlations to command line Jun 7, 2022
@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

@DidierRLopes @jmaslek can you review this PR? Thanks!

@stkerr stkerr marked this pull request as ready for review June 7, 2022 15:52
Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

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

So this is definitely a personal style thing, but I am not a fan of showing the whole correlation matrix.

I am fine with this being included behind an argparse argument that allows the user to specify whether they want the whole matrix or not

@jmaslek
Copy link
Collaborator

jmaslek commented Jun 7, 2022

Also the table crops off for me

Screen Shot 2022-06-07 at 12 05 38 PM

@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

@jmasley fair enough on the style point. I added a flag to allow for switching between either mode, defaulting to the current state. Picture below. I'll see what I can do about the text cropping for large matrices.

image

@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

Updated print logic to prevent truncating when there are lots of symbols. It should wrap to newlines to display all the values now, as shown in this picture:

image

@stkerr stkerr changed the title Remove masking for heatmap in hcorr and print correlations to command line Add argument to remove masking for heatmap in hcorr and print correlations to command line Jun 7, 2022
@stkerr stkerr requested a review from jmaslek June 7, 2022 19:18
@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

@jmaslek I've made some updates to address your feedback, so this is ready for another look.

@jmaslek jmaslek added the enhancement Enhancement label Jun 7, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Jun 7, 2022

Thanks for adding the flag. The print is still off. We have a custom method print_rich_table that does this formatting. Its in helper_funcs.py

I would also hide the console printing behind the raw flag. This one is actually easier to add. You can just add raw=True to parse_known_args_and_warn and it will auto add the --raw flag to the ns_parser namespace.

Screen Shot 2022-06-07 at 4 09 02 PM

@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

@jmaslek is this sort of what you had in mind? This feels more unreadable to me than the printed values before unless I misunderstood your previous comment.

image

@jmaslek
Copy link
Collaborator

jmaslek commented Jun 7, 2022

@jmaslek is this sort of what you had in mind? This feels more unreadable to me than the printed values before unless I misunderstood your previous comment.

image

Yeahhh that is what I had in mind. The actual terminal will be the limiting factor in how readable any table will be, so to keep with consistency across the code, can we keep the formatted table and then the user can change how many tickers they have loaded in.

May be overkill, but I could also envision a case where we only show print pairs that meet a criteria (--min-corr .5 --max-corr .9) but that is out of the scope here

@stkerr
Copy link
Contributor Author

stkerr commented Jun 7, 2022

@jmaslek got it, I'll go ahead and patch up the code I used to make the picture and get it submitted for review in a bit then.

May be overkill, but I could also envision a case where we only show print pairs that meet a criteria (--min-corr .5 --max-corr .9) but that is out of the scope here

That is an interesting idea. This would be helpful if it was integrated into some sort of scanner and also to look at variables beyond just the various parts of a candle - perhaps I would want to use it to scan for stocks that had a high volume correlation but had an inverse adjusted close correlation, as an example, to find a shortlist of stocks to dig into more.

@stkerr
Copy link
Contributor Author

stkerr commented Jun 8, 2022

@jmaslek can you take a look now? There is one failing Windows test, but that doesn't seem related to this change? The failing test is no longer failing after re-running the tests 🤷

@jmaslek
Copy link
Collaborator

jmaslek commented Jun 8, 2022

Awesome! This looks great. The last thing I will request is that the documentation under website/content/terminal gets updated to reflect the new arguments!

@jmaslek jmaslek merged commit a9a7771 into OpenBB-finance:main Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants