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

[python-package] Keep constants in sync with C++ library #4321

Closed
cyfdecyf opened this issue May 26, 2021 · 2 comments
Closed

[python-package] Keep constants in sync with C++ library #4321

cyfdecyf opened this issue May 26, 2021 · 2 comments

Comments

@cyfdecyf
Copy link
Contributor

Relevant only when #4089 is merged.

Summary

Find a way to keep constants in Python package in sync with C++ library.

This is needed in order to keep python package behaves the same as the cmdline version.

Motivation

#4089 handles data sampling in Python. It defines 2 constant variables ZERO_THRESHOLD and DEFAULT_BIN_CONSTRUCT_SAMPLE_CNT.

Currently they are set to the same value as in C++. If the default value changs in C++ side, we have to manually change them in Python side.

Description

I suggest we expose relevant constants in C API and use them in other language packages.

We can define a map which contains constant name and their value. Then add a C API to get those values, e.g.

const char* LGBM_GetConstant(const char* key);

References

Relevant code in #4089

Constants defined in pyhon package.
https://github.com/cyfdecyf/LightGBM/blob/f86d2db89d89810ddac0bc842af8d392a47edc85/python-package/lightgbm/basic.py#L21-L22

Usage:
https://github.com/cyfdecyf/LightGBM/blob/f86d2db89d89810ddac0bc842af8d392a47edc85/python-package/lightgbm/basic.py#L1556
https://github.com/cyfdecyf/LightGBM/blob/f86d2db89d89810ddac0bc842af8d392a47edc85/python-package/lightgbm/basic.py#L1217
https://github.com/cyfdecyf/LightGBM/blob/f86d2db89d89810ddac0bc842af8d392a47edc85/python-package/lightgbm/basic.py#L1580

@shiyu1994
Copy link
Collaborator

@cyfdecyf Thank you! Very good idea. And I believe it can be useful in the future.

@jameslamb
Copy link
Collaborator

Thank you for this excellent write-up! I've added this to #2302 with other feature requests, so I'll close this issue for now.

Anyone interested in contributing this is welcome to do so. Leave a comment below and the discussion can be re-opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants