-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[MNT] test cookbook notebooks #665
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
Conversation
|
/var/folders/_3/k_9k5d5n5zz57w7qfll9rzs40000gn/T/ipykernel_59786/3860718606.py:1: FutureWarning: YF.download() has changed argument auto_adjust default to True |
|
@fkiraly This is good to merge |
fkiraly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is going on here?
Your job passed:
https://github.com/PyPortfolio/PyPortfolioOpt/actions/runs/19291419407/job/55162781634
But mine fails with a line from the notebook. So was it not properly tested in your version of the job, or did the yfinance data change?
|
API of y finance has been updated many years ago. See the changes in the notebooks. IMHO yfinance should not be used at all. It's just pain. Just download once and commit csv files. Reproducible notebooks |
|
@fkiraly oh, you changed the notebooks back? The notebooks need those corrections. Replace "Adj Close" with "Close". It's a pity that jupyter notebooks and git don't work well together. Also, let's not work with a version of yfinance from 2018/2019... It's enough if you revert your last two commits |
100% agreed. That is what I would do if I had time to do this 😁 But this does not explain why your version of the notebook tests ran through and my version breaks with the expected API change. |
OOOOOHHHHHHH, yes, that is it. Stupid me - I will revert the notebook reverts. |
This reverts commit 168798a.
|
PS: may I kindly ask you to provide a description of the PR contents in your first post? That goes into the squashed commit message. If you are lazy, at least generate an AI description please, that takes 2 seconds. I know prolific contributors suffer from this syndrome but let's try? |
fkiraly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were right, I accidentally reverted your fixes to the notebooks.
Could you kindly update the PR description before we merge? So it is not empty?
|
@fkiraly Please merge. I wouldn't have the power to do so... |

Here we test all notebooks (e.g. we run them from top to bottom) with nbmake. nbmake is a package produced by our friend Alex Remedios, see https://github.com/treebeardtech/nbmake. Please note that we had to update the notebooks following a change in yfinance. We have replaced "Adj Close" with "Close".