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

Remove the use of dmlc::JSONWriter, dmlc::Stream, and dmlc serializer #289

Merged
merged 4 commits into from
Jul 11, 2021

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Jul 9, 2021

Extracted from #285.

  • Do not use dmlc/json.h, dmlc/io.h, and dmlc/serializer.h.
  • Replace all uses of dmlc::Stream with std::fstream and std::stringstream.
  • Replace dmlc::JSONWriter with rapidjson::Writer.
  • Use uint64_t for storing instance counts in JSON.
  • Rename TaskParameter to TaskParam, to be consistent with ModelParam.
  • Replace ReferenceSerialize with SerializeToJSON. Previously, ReferenceSerialize was a reference implementation of serializer for tree models. ReferenceSerialize was useful for verifying the correctness of other serializer methods of the tree objects. The reference serializer had to go, unfortunately, because it used dmlc serializer functions. As a replacement, I implemented a JSON serializer SerializeToJSON. While not very efficient compared to binary serializers, the JSON serializer will be useful for debugging and testing.
  • Reimplement LoadText with std::istream and std::getline.

@hcho3 hcho3 force-pushed the remove_dmlc_stream branch 2 times, most recently from a733551 to 37d5b13 Compare July 9, 2021 02:33
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #289 (88cae4e) into mainline (f769f5b) will decrease coverage by 0.84%.
The diff coverage is 93.03%.

Impacted file tree graph

@@              Coverage Diff               @@
##             mainline     #289      +/-   ##
==============================================
- Coverage       84.77%   83.92%   -0.85%     
  Complexity         46       46              
==============================================
  Files             102      102              
  Lines            7821     7889      +68     
  Branches           50       50              
==============================================
- Hits             6630     6621       -9     
- Misses           1166     1243      +77     
  Partials           25       25              
Impacted Files Coverage Δ
include/treelite/tree.h 97.77% <ø> (ø)
src/compiler/ast/ast.h 38.82% <ø> (ø)
src/compiler/ast/builder.h 100.00% <ø> (ø)
include/treelite/tree_impl.h 90.04% <50.00%> (-0.05%) ⬇️
src/frontend/xgboost.cc 80.08% <75.86%> (-0.94%) ⬇️
src/json_serializer.cc 92.17% <92.17%> (ø)
include/treelite/annotator.h 100.00% <100.00%> (ø)
src/annotator.cc 91.47% <100.00%> (+0.73%) ⬆️
src/c_api/c_api.cc 96.65% <100.00%> (ø)
src/compiler/ast/load_data_counts.cc 90.90% <100.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f769f5b...88cae4e. Read the comment docs.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Looked briefly. Not a complete review.

include/treelite/annotator.h Outdated Show resolved Hide resolved
src/frontend/xgboost.cc Outdated Show resolved Hide resolved
@hcho3 hcho3 merged commit 75b5574 into dmlc:mainline Jul 11, 2021
@hcho3 hcho3 deleted the remove_dmlc_stream branch July 11, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants