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

Jump to bottom

refactor param to replace JsonType internal #290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor param to replace JsonType internal #290

wants to merge 1 commit into from

Conversation

LHT129
Copy link
Collaborator

@LHT129 LHT129 commented Jan 2, 2025

No description provided.

@LHT129 LHT129 added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Jan 2, 2025
@LHT129 LHT129 self-assigned this Jan 2, 2025
@LHT129 LHT129 force-pushed the params branch 3 times, most recently from ed76d97 to 9d1a9a6 Compare January 3, 2025 02:26
@codecov Codecov
Copy link

codecov bot commented Jan 3, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Impacted file tree graph

@@            Coverage Diff            @@
##           main     #290       +/-   ##
=========================================
+ Coverage      0   49.61%   +49.61%     
=========================================
  Files         0      353      +353     
  Lines         0    32899    +32899     
  Branches      0     4136     +4136     
=========================================
+ Hits          0    16323    +16323     
- Misses        0    15717    +15717     
- Partials      0      859      +859

see 353 files with indirect coverage changes

jiaweizone
jiaweizone reviewed Jan 3, 2025
View reviewed changes
src/data_cell/flatten_datacell_parameter.h Outdated
IOParameterPtr io_parameter_{nullptr};
};

using FlattenDataCellParameterPtr = std::shared_ptr<FlattenDataCellParameter>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: FlattenDataCellParameterPtr -> FlattenDataCellParamPtr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/io/memory_block_io_parameter.h Outdated
uint64_t block_size_{};
};

using MemoryBlockIOParameterPtr = std::shared_ptr<MemoryBlockIOParameter>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: Name to long

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

inabao
inabao reviewed Jan 6, 2025
View reviewed changes
src/algorithm/hgraph_parameter.h

bool use_reorder_{false};
uint64_t ef_construction_{400};
uint64_t build_thread_count_{100};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Set the default value to the value in options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to do one thing in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

fine

src/data_cell/flatten_datacell_test.cpp Outdated
@@ -19,92 +19,60 @@
#include <utility>

#include "catch2/catch_template_test_macros.hpp"
#include "catch2/generators/catch_generators.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

use <> to include external libraries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/algorithm/hgraph_parameter.h
uint64_t ef_construction_{400};
uint64_t build_thread_count_{100};

std::string name_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

struct's public variables do not end with an _

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how to distinguish between local variables and member variables

src/data_cell/flatten_interface.h Outdated
@@ -18,6 +18,7 @@
#include <nlohmann/json.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to include the header?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/data_cell/graph_datacell_parameter.cpp Outdated

#include "graph_datacell_parameter.h"

#include "fmt/format-inl.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/data_cell/graph_datacell_test.cpp Outdated
#include "catch2/catch_template_test_macros.hpp"
#include "catch2/generators/catch_generators.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/quantization/quantizer_parameter.cpp Outdated

#include "quantizer_parameter.h"

#include "fmt/format-inl.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/quantization/quantizer_parameter.h
explicit QuantizerParameter(std::string name);

private:
std::string name_{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

src/quantization/fp32_quantizer.h
@@ -17,6 +17,7 @@
#include <cstdint>
#include <cstring>

#include "fp32_quantizer_parameter.h"
#include "index/index_common_param.h"
#include "inner_string_params.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

#include "src/index/inner_string_params.h"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no need src

src/quantization/fp32_quantizer_parameter.cpp

#include "fp32_quantizer_parameter.h"

#include "inner_string_params.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

@LHT129 LHT129 force-pushed the params branch 6 times, most recently from 5d38e65 to e04138f Compare January 7, 2025 11:46
jiaweizone
jiaweizone approved these changes Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

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

LGTM

@LHT129 LHT129 force-pushed the params branch 2 times, most recently from d4f7e42 to dd80315 Compare January 9, 2025 01:11
inabao
inabao approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@inabao inabao left a comment

Choose a reason for hiding this comment

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

LGTM

@LHT129 LHT129 force-pushed the params branch 2 times, most recently from 8ff01de to c3eb8bc Compare January 9, 2025 12:01
wxyucs
wxyucs approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inabao inabao inabao approved these changes

@wxyucs wxyucs wxyucs approved these changes

@jiaweizone jiaweizone jiaweizone approved these changes

@ShawnShawnYou ShawnShawnYou Awaiting requested review from ShawnShawnYou ShawnShawnYou is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@LHT129 LHT129

Labels
kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LHT129 @jiaweizone @wxyucs @inabao