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.

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

small code and docs refactoring #3681

Merged
merged 7 commits into from
Dec 28, 2020
Merged

small code and docs refactoring #3681

merged 7 commits into from
Dec 28, 2020

Conversation

StrikerRUS
Copy link
Collaborator

No description provided.

# Ignore unknown #pragma warning
if((CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
OR (CMAKE_CXX_COMPILER_ID STREQUAL "GNU"))
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be set after -Wall to take effect.

@@ -272,6 +266,9 @@ if(UNIX OR MINGW OR CYGWIN)
if(USE_SWIG)
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-strict-aliasing")
endif()
if(NOT USE_OPENMP)
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas -Wno-unused-private-field")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hide

/Users/runner/work/1/s/src/objective/binary_objective.hpp:208:14: warning: private field 'deterministic_' is not used [-Wunused-private-field]
  const bool deterministic_;
             ^

with AppleClang.

@@ -408,6 +400,14 @@ LIGHTGBM_C_EXPORT int LGBM_DatasetAddFeaturesFrom(DatasetHandle target,

// --- start Booster interfaces

/*!
* \brief Get boolean representing whether booster is fitting linear trees.
* \param handle Handle of booster
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be booster, not dataset.

if (linear_tree) {
if (device_type == std::string("gpu")) {
if (device_type != std::string("cpu")) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To generalize for possible future new enum options.

@@ -600,7 +600,6 @@ Dataset* DatasetLoader::LoadFromBinFile(const char* data_filename, const char* b
}
mem_ptr = buffer.data();
const float* tmp_ptr_raw_row = reinterpret_cast<const float*>(mem_ptr);
std::vector<float> curr_row(dataset->num_numeric_features_, 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused.

@@ -211,7 +211,7 @@ void LinearTreeLearner::CalculateLinear(Tree* tree, bool is_refit, const score_t
} else {
raw_features = tree->branch_features(i);
}
std::sort(raw_features.begin(), raw_features.end());
std::stable_sort(raw_features.begin(), raw_features.end());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about this. Refer to #2154 (comment) and #1739.
@btrotta Is there a possibility to meet equivalent elements in raw_features?
cc @guolinke

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is not need for stable-sort. I think here is only to get unique feature indices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@guolinke
Thanks! Reverted in 4358b4a.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

changes look good to me! But @btrotta should approve before we merge

@StrikerRUS StrikerRUS merged commit 5a46084 into master Dec 28, 2020
@StrikerRUS StrikerRUS deleted the small_cleanup branch December 28, 2020 22:24
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants