pytorch-lr-scheduler
pytorch-lr-scheduler copied to clipboard
Source code decoupling
It would be better to restruct code and publish a kind of interface in order to everyone can use it as dependency. There are several issues.
- Separate entry point in order to use deepwalk in different contexts.
- Provide option to compile without OpenMP. Decoration of
omp_get_thread_num()
call with_OPENMP
macro wil be nice. - Add some tests. For example, death test can help to detect wrong memory writes.
- There is shared inner state that prevent usage as is in concurrent runtime.
The first two points are the most crucial for me since I would like to use it in side project.
In the arbitrary order:
- Doing that will ruin portability for Windows, as there is not -openmp-simd flag that I would have used otherwise on gcc, clang and icc. Besides, what is wrong with openmp dependency? It is supported by literally any compiler nowadays.
- I am not sure if this is entirely possible to decouple rng state without serious performance degradation. The code is optimized for speed now, it is not really worth to lose it.
- After the fix, there seems no memory errors on the graphs I have. It is pretty unreasonable to test other functions than hsm.
- It is related to [4]. Until [4] is done, I see little reason for that.
I just do not see the benefits of [3], and [4,1] seem to have little impact on other users.
Well I see. It is up to you.
- The reason that it is. OpenMP sometimes makes debugging much harder and inject external dependencies.
- Nevertheless it is possible even though you have not tried or won't try.
- Memory errors are only one side the other is correctness. How do you prove that implementation unerring? By the way, what
testing interface should be in master
mean here #4? - I misunderstand this point totally.
What kind of users are you talking about? I am user. It have large impact on me.
-
It seems like OpenMP is supported by most sensible and few non-sensible compilers. I would not really agree that OpenMP is too much of an external dependency as so many compilers support it natively. Going for pthreads would break Windows support, going std::parallel would break any compiler not happy with c++17. I did not find the proper way to fully disable openmp without losing FMA speedup on the update loop: maybe there is. If you find one, I will be happy to merge.
-
I did try to abstract the RNG, the performance dropped significantly. There is a reason things are, even in the RNG choice. Yes, using something like taus88 would be more "clean", yet the program loses in speed.
-
I have verified the performance on the classification tasks on the datasets from the original paper (download link). #4 is not correct.
[5] Was referring to [1]. The intended usage of the tool is to produce an embedding of a graph. I di not really see how coupled code hinders the functionality.