Skip to content

Conversation

@gionanide
Copy link

No description provided.

@albertauyeung
Copy link
Owner

Hi @gionanide , thank you for this contribution! I left a few comments in the code. Also, do you have any benchmark showing how using GPU can speed up the training process? It would be good to see some numbers in the readme file!

])


#because there are many dot products to calculate during the procedure, GPU can be used for this task
Copy link
Owner

Choose a reason for hiding this comment

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

I think this part can be simplified to as follows, because this is not an actual script that can be executed, but just a sample of how to use the module:

gpu = False   # Set to True to use GPU in training and prediction

Copy link
Author

Choose a reason for hiding this comment

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

Yes you are right it is more clear like this, and also according to its significance it is better in the end of the arguments, as you proposed.

@@ -1,9 +1,15 @@
import pycuda.autoinit
import pycuda.gpuarray as gpuarray
import skcuda.linalg as linalg
Copy link
Owner

Choose a reason for hiding this comment

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

It would be good if you can add a requirements.txt file to specify the dependencies. I didn't have one because it was only dependent on numpy before.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will write a .txt file like this and upload it.

@gionanide
Copy link
Author

Hello Albert @albertauyeung Because the matrix R in very small, the running time with and without GPU is the same, to see the difference you need to have bigger matrices. I can add in the read me file as comment the dimensions of a bigger array and the running times to illustrate the difference if you want.

@albertauyeung
Copy link
Owner

Looks good. Thanks! I will test and then merge soon.

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.

2 participants