Skip to content

Conversation

@trivialfis
Copy link
Member

@trivialfis trivialfis commented Nov 2, 2025

  • Document the category container format.
  • Move out the JSON schema to avoid filling the document with a long JSON document.

I haven't included the category container in the JSON schema yet. Looking back, the schema doesn't seem to be particularly useful.

Ref: dmlc/treelite#639

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances documentation for XGBoost's JSON serialization format and improves code documentation. The changes focus on explaining the category encoding mechanism introduced in version 3.1 and reorganizing schema documentation.

  • Updated documentation comment style in json.h header with stability note
  • Added comprehensive documentation for the categories JSON format including feature_segments, sorted_idx, and encoding schemas
  • Refactored JSON schema documentation into a separate file with cross-references

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
include/xgboost/json.h Updated ValueKind enum documentation to use Doxygen format and added stability note
doc/tutorials/saving_model.rst Added table of contents, new Categories section explaining serialization format, and replaced inline schema with document reference
doc/model_schema.rst New file created to hold JSON model schema content previously embedded inline

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@trivialfis trivialfis requested a review from hcho3 November 2, 2025 17:53
Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

Can you update model.schema to include the cats field?

@trivialfis
Copy link
Member Author

Can you update model.schema to include the cats field?

Do you use it?

@hcho3
Copy link
Collaborator

hcho3 commented Nov 4, 2025

No, but other users may decide to use the JSON schema file.

@trivialfis
Copy link
Member Author

but other users may decide to use the JSON schema file.

For context, I'm inclined to remove it.

@hcho3
Copy link
Collaborator

hcho3 commented Nov 4, 2025

Feel free to remove it

@trivialfis
Copy link
Member Author

Removed.

@trivialfis trivialfis requested a review from hcho3 November 4, 2025 07:07
@trivialfis trivialfis changed the title [doc] Document the JSON schema for the categories container. [doc] Document the categories container, remove JSON schema. Nov 4, 2025
@trivialfis trivialfis merged commit c5ba21d into dmlc:master Nov 4, 2025
96 of 100 checks passed
@trivialfis trivialfis deleted the cat-model-doc branch November 4, 2025 08:01
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