Correct conversions errors from u64 to size_t(u32 on msvc32) #104
Conversation
src/rnn/mikolov_rnn.cc
Outdated
| data_->memmgr.initialize(pageSize); | ||
| data_->alloc = data_->memmgr.value().core(); | ||
| auto& alloc = data_->alloc; | ||
|
|
||
| const auto embedding_size = static_cast<size_t>(hdr.layerSize) * static_cast<size_t>(hdr.vocabSize); | ||
| const auto matrix_size = static_cast<size_t>(hdr.layerSize) * static_cast<size_t>(hdr.layerSize); |
There was a problem hiding this comment.
I had second thought about whether hdr.layerSize * hdr.layerSize is mistake or not...
There was a problem hiding this comment.
It's a square matrix layerSize times layerSize, so that's correct
src/rnn/mikolov_rnn.cc
Outdated
| data_->memmgr.initialize(pageSize); | ||
| data_->alloc = data_->memmgr.value().core(); | ||
| auto& alloc = data_->alloc; | ||
|
|
||
| const auto embedding_size = static_cast<size_t>(hdr.layerSize) * static_cast<size_t>(hdr.vocabSize); |
There was a problem hiding this comment.
Could you name this embedding_matrix_size
Usually, a vector representation for a single token is called embedding.
Next thing should be called transition_matrix_size in that case.
There was a problem hiding this comment.
transition_matrix_size as well please!
Anyway, congratulations on 1000th commit :p
There was a problem hiding this comment.
Oh sorry, I thought you meant it as if we leave name as it is
There was a problem hiding this comment.
Sorry for the confusion.
Need to improve my clearness.
There was a problem hiding this comment.
Addressed.
And thanks, sorry about ruining nice number 999 😄
DoumanAsh
force-pushed
the
fix_msvc32_errors
branch
from
40f5a20 to
091287c
Compare
|
Since build will be fixed by this commit, it would be nice if you could try to add msvc32 build to appveyor just to make sure it is not going to get broken in future By default cmake choose x86 msbuild generator, unless you specify it, as far as my experience goes |
|
https://ci.appveyor.com/project/eiennohito/jumanpp/builds/20694567/job/u883gac1of6chfnj Some tests fail; I won't enable it by default. |
|
Oh, let me re-check it, I remember I had no troubles yesterday |
There are still lots of warnings due to conversions between 32/64 bit integers, but I fixed only errors and these warnings around them.
Users should be able build it with msvc32 compiler
Closes #91