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] Add base-1 indexing option for prediction outputs #4970

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

david-cortes
Copy link
Contributor

@david-cortes david-cortes commented Jan 23, 2022

ref #4968

The prediction function in the R package produces outputs that have a base-0 numeration. R uses base-1 numeration however, which makes the result a bit confusing and harder to use on the outside.

What's even more confusing, it applies base-0 numeration also to start_iteration without any warning about it either.

This PR adds an option to make the inputs and result have a base-1 numeration instead, and along the way corrects type None (which does not exist in R) in the docs about these parameters to say NULL.

@david-cortes
Copy link
Contributor Author

The mac CI failures are due to dependencies that failed to install and not related to this PR.

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.

I haven't forgotten about this PR, but I'm not ready to review it yet. There are many places in the R package which deal with references to numeration (i.e. where users have to provide a number to refer to "the nth thing"), and those references use a mix of 1-based and 0-based indexing.

Before considering this proposal to control that behavior with a boolean flag in one limited place, I'd like to list out all such places in the package's public API and see if we can come up with a general standard for when the R package uses 1-based numeration and when it uses 0-based numeration.

I'll try to complete that list and open an a "request for comment (RFC)" issue with a proposal soon.

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

Successfully merging this pull request may close these issues.

None yet

2 participants