Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct conversions errors from u64 to size_t(u32 on msvc32) #104

Merged
merged 2 commits into from
Dec 1, 2018
Merged

Correct conversions errors from u64 to size_t(u32 on msvc32) #104

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9d1d38
Correct conversions errors from u64 to size_t(u32 on msvc32)
DoumanAsh Nov 30, 2018
091287c
Apply comments
DoumanAsh Dec 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,6 @@ VERSION
__pycache__/

# Mac OS
.DS_Store
.DS_Store

build/
4 changes: 2 additions & 2 deletions src/core/analysis/rnn_scorer_gbeam.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,9 @@ Status RnnScorerGbeamFactory::make(StringPiece rnnModelPath,
state_->resolver.build(dic, state_->config, state_->rnnReader.words()));
auto& h = state_->rnnReader.header();
state_->embeddings = {state_->rnnReader.embeddings(), h.layerSize,
h.vocabSize};
static_cast<jumanpp::size_t>(h.vocabSize)};
state_->nceEmbeddings = {state_->rnnReader.nceEmbeddings(), h.layerSize,
h.vocabSize};
static_cast<jumanpp::size_t>(h.vocabSize)};
if (h.layerSize > 64 * 1024) {
return JPPS_NOT_IMPLEMENTED << "we don't support embed sizes > 64k";
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/analysis/score_processor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ util::ArraySlice<BeamCandidate> processBeamCandidates(
candidates = util::MutableArraySlice<BeamCandidate>{candidates, 0, sz};
}
std::sort(candidates.begin(), candidates.end(), comp);
auto size = std::min<u64>(maxBeam, candidates.size());
auto size = std::min<jumanpp::size_t>(maxBeam, candidates.size());
return util::ArraySlice<BeamCandidate>{candidates, 0, size};
}

Expand Down
22 changes: 13 additions & 9 deletions src/rnn/mikolov_rnn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,29 +177,33 @@ Status MikolovModelReader::parse() {
auto maxBlock = std::max<u64>({hdr.layerSize * hdr.vocabSize, hdr.maxentSize,
hdr.layerSize * hdr.layerSize});
// 3 comes from rounding to next value + sizeof(float)
auto pageSize = 1ULL << (static_cast<u32>(std::log2(maxBlock)) + 3);
auto pageSize = static_cast<size_t>(1) << (static_cast<u32>(std::log2(maxBlock)) + 3);
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);
Copy link
Contributor

@eiennohito eiennohito Dec 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it

Copy link
Contributor

Choose a reason for hiding this comment

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

transition_matrix_size as well please!

Anyway, congratulations on 1000th commit :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I thought you meant it as if we leave name as it is

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion.

Need to improve my clearness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

And thanks, sorry about ruining nice number 999 😄

const auto matrix_size = static_cast<size_t>(hdr.layerSize) * static_cast<size_t>(hdr.layerSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had second thought about whether hdr.layerSize * hdr.layerSize is mistake or not...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a square matrix layerSize times layerSize, so that's correct


data_->embeddingData =
alloc->allocateBuf<float>(hdr.layerSize * hdr.vocabSize, 64);
alloc->allocateBuf<float>(embedding_size, 64);
data_->nceEmbeddingData =
alloc->allocateBuf<float>(hdr.layerSize * hdr.vocabSize, 64);
alloc->allocateBuf<float>(embedding_size, 64);
data_->matrixData =
alloc->allocateBuf<float>(hdr.layerSize * hdr.layerSize, 64);
data_->maxentWeightData = alloc->allocateBuf<float>(hdr.maxentSize, 64);
alloc->allocateBuf<float>(matrix_size, 64);
data_->maxentWeightData = alloc->allocateBuf<float>(static_cast<size_t>(hdr.maxentSize), 64);

JPP_RIE_MSG(copyArray(contents, data_->embeddingData,
hdr.layerSize * hdr.vocabSize, &start),
embedding_size, &start),
"embeds");
JPP_RIE_MSG(copyArray(contents, data_->nceEmbeddingData,
hdr.layerSize * hdr.vocabSize, &start),
embedding_size, &start),
"nce embeds");
JPP_RIE_MSG(copyArray(contents, data_->matrixData,
hdr.layerSize * hdr.layerSize, &start),
matrix_size, &start),
"matrix");
JPP_RIE_MSG(
copyArray(contents, data_->maxentWeightData, hdr.maxentSize, &start),
copyArray(contents, data_->maxentWeightData, static_cast<size_t>(hdr.maxentSize), &start),
"maxent weights");
if (start != contents.size()) {
return Status::InvalidState() << "did not read rnn model file fully";
Expand Down