Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

Minor improvements #9

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Minor improvements #9

wants to merge 8 commits into from

Conversation

goshaQ
Copy link

@goshaQ goshaQ commented Apr 14, 2019

What was fixed?

This PR addresses the following issues: #4 #8

How?

Now the model takes into consideration the current values of driving series.
Also resolved the issue with running on GPU.

Comment

Btw, it is strange that in the original implementation the current values of exogenous (driving) series are not considered, because they are not dependent on the target values and usually are known in advance.
But as for me, it depends on the problem formulation.

Copy link

@FutureGoingOn FutureGoingOn left a comment

Choose a reason for hiding this comment

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

Thank you for your guidance, but I didn't see a significant increase in speed. NVIDIA information showed that GPU memory usage increased from about 1100MB to about 1200MB. I worked on Pytorch 1.0.1 and Pytorch 0.4.0.
I also can't understand that all variables are in CUDA but GPU memory usage has not been improved.

@goshaQ
Copy link
Author

goshaQ commented Apr 16, 2019

The current version of the model throws a TypeError if you try to run it with Pytorch 1.x. The problem is that the model don't work on GPU because it allocates regular FloatTensor instead of cuda.FloatTensor. In this PR I fixed it. The latter is not an optimized version of the former, the difference between them that one of them occupies CPU memory while the other GPU memory (roughly speaking). So there should not be any improvement in terms of memory usage and speed.

Increased usage of GPU memory can be explained by increased batch size. Now it takes into consideration the current values of driving series. The author of the reference implementation though that it might be meaningless to make predictions based on this values and excluded them, however this is wrong.

In the original paper, the authors tried to predict the NASDAQ100 index by using stock prices of only 81 company. Yes, this index is a simple linear combination of stock prices of a hundred of the largest companies, but they don't use stock prices of all contributing companies, so it isn't a "cheating".

@FutureGoingOn
Copy link

@goshaQ
Thanks for your reply. But I still can't improve the speed on Pytorch 0.4.0, and the memory of GPU is not well utilized. Is this a code problem? Or is it a problem from the delay of data sending and receiving? At least the memory upper limit of the GPU should be reached, and the GPU utilization is also only about 30%. The speed of the model was unacceptable when my data volume increased a lot.

Btw, I totally agree with you about the NARX model and DARNN. Different forecasting methods can be applied to different scenarios, and the forecasting accuracy will be higher in this method.

isaacmg added a commit to AIStream-Peelout/flow-forecast that referenced this pull request Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants