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

[R-package] Handle integer types more accurate in R-to-C interface #4291

Merged
merged 33 commits into from
May 15, 2021

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented May 14, 2021

Fix compilation warnings like the following one

C:\Users\runneradmin\RtmpMzXGiG\R.INSTALL11384d93526b\lightgbm\src\src\lightgbm_R.cpp(670,3): warning C4244: 'argument': conversion from 'int64_t' to 'int', possible loss of data [C:\Users\runneradmin\RtmpMzXGiG\R.INSTALL11384d93526b\lightgbm\src\build\_lightgbm.vcxproj]

Checked all Rf_asInteger and INTEGER calls and tried to bring data types in the consistency with original ones from
https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/c_api.h.

@StrikerRUS
Copy link
Collaborator Author

Cannot verify proposed changes due to

Setting up MiKTeX
miktexsetup.exe: Couldn't connect to server
miktexsetup.exe: Data: code="7", url="https://api2.miktex.org/hello"

and those compilation warnings were presented only in MSVC jobs. So leaving this PR in a draft for some time to let MiKTeX get healthy.

@jameslamb
Copy link
Collaborator

oh no! I can at least pull this PR and test things in my local environment with R. Hopefully the MiKTeX issues are resolved quickly this time 😬

@StrikerRUS
Copy link
Collaborator Author

Thank you very much! I remember you don't have an easy access to Windows environment. I think there is no rush at all.

@StrikerRUS
Copy link
Collaborator Author

Ha, look! MSVC R-package jobs are already green! Let me check logs right now then.

@StrikerRUS
Copy link
Collaborator Author

Tail ( because there are dozens of warnings from Eigen library) of the compilation logs from current master's r-package (windows-2016, MSVC, R 3.6, cmake) CI job:

  tree_learner.cpp


  voting_parallel_tree_learner.cpp


  c_api.cpp


  lightgbm_R.cpp


C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\src\lightgbm_R.cpp(677): warning C4244: 'argument': conversion from 'int64_t' to 'int', possible loss of data [C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\build\_lightgbm.vcxproj]


C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\src\lightgbm_R.cpp(681): warning C4244: 'argument': conversion from 'int64_t' to 'int', possible loss of data [C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\build\_lightgbm.vcxproj]


C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\src\lightgbm_R.cpp(700): warning C4244: 'argument': conversion from 'int64_t' to 'int', possible loss of data [C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\build\_lightgbm.vcxproj]


C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\src\lightgbm_R.cpp(704): warning C4244: 'argument': conversion from 'int64_t' to 'int', possible loss of data [C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\build\_lightgbm.vcxproj]


     Creating library C:/Users/runneradmin/RtmpaswVvL/R.INSTALL116c16fe3eaf/lightgbm/src/Release/lib_lightgbm.lib and object C:/Users/runneradmin/RtmpaswVvL/R.INSTALL116c16fe3eaf/lightgbm/src/Release/lib_lightgbm.exp


  _lightgbm.vcxproj -> C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf\lightgbm\src\Release\lib_lightgbm.dll


Found library file: C:\Users\runneradmin\RtmpaswVvL\R.INSTALL116c16fe3eaf/lightgbm/src/Release/lib_lightgbm.dll to move to D:/a/LightGBM/LightGBM/RLibrary/00LOCK-lightgbm/00new/lightgbm/libs/x64

Removing 'build/' directory

And the same from this PR:

  tree_learner.cpp


  voting_parallel_tree_learner.cpp


  c_api.cpp


  lightgbm_R.cpp


     Creating library C:/Users/runneradmin/RtmpSU9vCA/R.INSTALLa1c49706a09/lightgbm/src/Release/lib_lightgbm.lib and object C:/Users/runneradmin/RtmpSU9vCA/R.INSTALLa1c49706a09/lightgbm/src/Release/lib_lightgbm.exp


  _lightgbm.vcxproj -> C:\Users\runneradmin\RtmpSU9vCA\R.INSTALLa1c49706a09\lightgbm\src\Release\lib_lightgbm.dll


Found library file: C:\Users\runneradmin\RtmpSU9vCA\R.INSTALLa1c49706a09/lightgbm/src/Release/lib_lightgbm.dll to move to D:/a/LightGBM/LightGBM/RLibrary/00LOCK-lightgbm/00new/lightgbm/libs/x64

Removing 'build/' directory

@StrikerRUS StrikerRUS marked this pull request as ready for review May 14, 2021 22:20
@StrikerRUS
Copy link
Collaborator Author

StrikerRUS commented May 14, 2021

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/843596563

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-9274e817e3e94137a1df72872ac00067
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-99be5b784342430ba915d75b3eae5c26
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

@StrikerRUS
Copy link
Collaborator Author

StrikerRUS commented May 14, 2021

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/843596979

Status: success ✔️.

@jameslamb
Copy link
Collaborator

I remember you don't have an easy access to Windows environment. I think there is no rush at all.

Ha yes, I have a laptop from 2014 running Windows 10. The only things I have on it are things for use on this project: Visual Studio, CMake, git, RStudio, R, and Rtools, but it usually takes 20-30 minutes of installing updates when I turn it on, just because I use it so infrequently.

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.

This is a nice cleanup, thank you! I checked logs and don't see these warnings any more.

By the way, this made me think. We should have done this a LONG time ago and I apologize for not doing it sooner...could you add your name to the list of co-authors at https://github.com/microsoft/LightGBM/blob/c629cb0b6bd2830894b710cbd4d8241b82ac3105/R-package/DESCRIPTION? Up to you if you'd like to do it in this PR or a separate one, either is ok with me.

@StrikerRUS StrikerRUS merged commit 90677e4 into master May 15, 2021
@StrikerRUS StrikerRUS deleted the r_int_cast branch May 15, 2021 13:08
@StrikerRUS
Copy link
Collaborator Author

could you add your name to the list of co-authors

Oh, thank you!
Could you please advise what type of role should I choose? ctb?

"ctb"
(Contributor) Use for authors who have made smaller contributions (such as code patches etc.) but should not show up in the package citation.

@jameslamb
Copy link
Collaborator

@StrikerRUS I would support either aut or ctb, whichever you are more comfortable with.

I think you have made more contributions to the R package than others who are already listed as aut in that file, and that aut is more appropriate.

From https://journal.r-project.org/archive/2012-1/RJournal_2012-1_Hornik~et~al.pdf

"aut" (Author): Full authors who have made substantial contributions to the package and
should show up in the package citation.

@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 23, 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.

2 participants