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

[ci] [R-package] add unit tests on monotone constraints #4352

Merged
merged 8 commits into from
Jun 15, 2021

Conversation

jameslamb
Copy link
Collaborator

Contributes to #4345, see #4346 (comment).

This PR proposes adding unit tests on monotone constraints to the R package, since there currently are not any.

This test is basically a translation of the corresponding Python test into R, but simplified by leaving out details about interaction constraints. The goal of this PR is to have at least basic coverage of monotone constraints from the R package.

def test_monotone_constraints(test_with_interaction_constraints):

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jun 9, 2021

/gha run r-valgrind

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

Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jun 9, 2021

/gha run r-valgrind

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

Status: failure ❌.

@jameslamb
Copy link
Collaborator Author

@StrikerRUS I wanted to test the results of the valgrind job when removing testthat::skip() calls from the R package's tests, to compare it to the results I'm seeing in #4353. I'm doing that here on this unrelated PR, with #4352 (comment), because I couldn't get the optional checks working on my fork.

Could you please tell me what secrets I need to set up on my fork to be able to run the triggering_comments jobs there, like you did in #3740 (comment)?

I tried setting a secret SECRETS_WORKFLOW in the settings on my fork to a GitHub personal access token and to the client token for a GitHub oauth app. In both cases, when I left a comment to trigger valgrind (jameslamb#43) the jobs failed with this error:

{
"message": "Bad credentials",
"documentation_url": "https://docs.github.com/rest"
}

Thank you!

@jameslamb
Copy link
Collaborator Author

Ok I've reverted the temporary change from 2714777, and this is now ready for review again.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Jun 9, 2021

/gha run r-valgrind

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

Status: success ✔️.

@StrikerRUS
Copy link
Collaborator

@jameslamb

Could you please tell me what secrets I need to set up on my fork

You need to set one secret named WORKFLOW with your PAT with at least repo:status and workflow scopes (selecting workflow auto-chooses the whole repo section unfortunately).

image

After that don't forget to replace all microsoft/LightGBM occurrences with jameslamb/LightGBM in repo's code, e.g. here

repository: microsoft/LightGBM

"${GITHUB_API_URL}/repos/microsoft/LightGBM/dispatches"

@jameslamb
Copy link
Collaborator Author

aha!!

After that don't forget to replace all microsoft/LightGBM occurrences with jameslamb/LightGBM in repo's code, e.g. here

This was the part I was missing. I'll try that, thank you very much.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but I leaved some comments that highlight the difference with the original test. I think that it's OK to have not two identical tests for R and Python.

Also, I have one comment about how to speed up the test.

R-package/tests/testthat/test_basic.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_basic.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_basic.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_basic.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_basic.R Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

Tests started failing after a15596a, which exposed a few more places where these tests differed from the Python equivalents.

More changes in 4950ed3 to make these tests consistent with the Python ones.

  1. Replace seq_len(n) / n with code that produces an output identical to numpy.linspace()
  2. train for 100 rounds, not 10

This is ready for another review.

@jameslamb jameslamb merged commit a592316 into master Jun 15, 2021
@jameslamb jameslamb deleted the ci/monotone-tests branch June 15, 2021 15:46
@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