Don't copy more than has been allocated to device_features. #3752
Conversation
ChipKerchner
requested review from
btrotta,
chivee and
guolinke
as code owners
|
@guolinke @shiyu1994 @btrotta Could you please help to review this tiny PR with #3750? I'd like to start improving CUDA CI part ASAP but I can't without two these PRs are merged. Sorry for the rush, thanks! |
| @@ -408,7 +408,7 @@ void CUDATreeLearner::copyDenseFeature() { | |||
| // looking for dword_features_ non-sparse feature-groups | |||
| if (!train_data_->IsMultiGroup(i)) { | |||
| dense_feature_group_map_.push_back(i); | |||
| auto sizes_in_byte = train_data_->FeatureGroupSizesInByte(i); | |||
| auto sizes_in_byte = std::min(train_data_->FeatureGroupSizesInByte(i), static_cast<size_t>(num_data_)); | |||
There was a problem hiding this comment.
should we assert for the type for FeatureGroupData ? I think it should be 1-Byte type.
There was a problem hiding this comment.
I don't think an assert would be meaningful since FeatureGroupSizesInByte is returning a padded size (at least after 3.0.x it is).
There was a problem hiding this comment.
@guolinke Can we merge or this discussions should be resolved before that?
There was a problem hiding this comment.
I mean that using static_cast<size_t>(num_data_) as the size, the type should be 1-Byte type, right?
FeatureGroup could be 4-bit, 1Byte, 2Byte and 4Byte.
However, I think we can merge this first.
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
@StrikerRUS Don't copy more than has been allocated to device_features.
Fix CUDA failures #3450
Possible compilation issues related to #3750