Skip to content

RowGroupColumnName of ranking trainers options class defaults to null #3365

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

Closed
artidoro opened this issue Apr 16, 2019 · 3 comments
Closed
Labels
API Issues pertaining the friendly API bug Something isn't working P3 Doc bugs, questions, minor issues, etc.

Comments

@artidoro
Copy link
Contributor

artidoro commented Apr 16, 2019

The Options class of the ranking trainers (FastTree and LightGbm) defaults to RowGroupColumnName = null.

This is:

  1. Inconsistent with the simple constructor where RowGroupColumnName defaults to GroupId
  2. Not desirable as in ranking the row group is very important for correct training

Here are the lines where the row group column name is set:

/// <summary>
/// Column to use for example groupId.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Column to use for example groupId", ShortName = "groupId", SortOrder = 5, Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)]
public string RowGroupColumnName = null;

We need to update the default and align it with the simple constructor.

@artidoro artidoro added the bug Something isn't working label Apr 16, 2019
@codemzs codemzs added the P0 Priority of the issue for triage purpose: IMPORTANT, needs to be fixed right away. label May 22, 2019
@artidoro
Copy link
Contributor Author

I am afraid that this will require a breaking API change which at this point we can't do. This would be something that we should consider changing for 2.0.

@artidoro artidoro added breaking and removed P0 Priority of the issue for triage purpose: IMPORTANT, needs to be fixed right away. labels May 29, 2019
@artidoro artidoro removed their assignment May 29, 2019
@justinormont
Copy link
Contributor

I would expect the ranking trainer to throw if it doesn't have a RowGroupColumnName defined.

Hence, I don't see this as a breaking change, since without it the user's pipeline (even if it runs) is broken as the result won't have utility. This questions our definition of a breaking API change.

@antoniovs1029 antoniovs1029 added API Issues pertaining the friendly API P3 Doc bugs, questions, minor issues, etc. labels Jan 9, 2020
@najeeb-kazmi
Copy link
Member

Tracking in #4749.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API bug Something isn't working P3 Doc bugs, questions, minor issues, etc.
Projects
None yet
Development

No branches or pull requests

5 participants