Skip to content

Edoardo#14

Open
edoardomocchi wants to merge 13 commits intomainfrom
Edoardo
Open

Edoardo#14
edoardomocchi wants to merge 13 commits intomainfrom
Edoardo

Conversation

@edoardomocchi
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@ErTucano ErTucano left a comment

Choose a reason for hiding this comment

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

Apart for the comment I added it looks all good to me. I'm not approving because I'm waiting for feedback from Giulia about the changes in .gitignore and .gitmodules.


train_dataset = timeseries_dataset_from_array(
data=data[:train_size],
targets=data[input_sequence_length:train_size + input_sequence_length, 0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure input_sequence_length:train_size + input_sequence_length stays in the bounds of the data DataFrame?

Copy link
Collaborator

@ErTucano ErTucano left a comment

Choose a reason for hiding this comment

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

For when you do the granger_analysis in granger.ipynb can you modify the function so that it doesn't print the correlation between the same tickers (like SIRI and SIRI or CTSH and CTSH).

About the ipynb file you have recently added can you also put all the import in a single cell? I noticed you repeated them every time. It shouldn't be necessary; in case someone where to copy the code on vscode it would look bad having multiple imports all around.

There are also some errors in the code, as you see from the exceptions (the red text in the output), if you can please resolve it.

Can you answer me about the question I asked on the previous comment on models/lstm.py?

As a refernce look at the ipynb files uploaded by Riccardo, they are well organised and tidy.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants