lishiwei2011 / gradientboostedmodels

Automatically exported from code.google.com/p/gradientboostedmodels
0 stars 0 forks source link

if the first observation's offset = 0, then the offset will be ignored. #6

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Create a dataset with 1st observation's offset = 0
2. Build two gbm models; one with offset and the other without offset.
3. The result is identical.
4. If you reshuffle the data so that 1st observation != 0, the two models will 
be different.

What is the expected output? What do you see instead?
expect models with and without offset will be different, instead they are the 
same.

What version of the product are you using? On what operating system?
gbm 1.6-3.1 

Please provide any additional information below.

The problem is this line in gbm.fit:

If(is.null(offset) || (offset == 0))
{
offset <- NA
}

Here offset is a vector. Offset == 0 is evaluated to a vector of TRUE and FALSE 
values. When the condition inside an if-clause has length > 1, only the first 
element will be used. So if offset[1] == 0, then the condition is TRUE, and 
offset <- NA.

I am guessing Greg Ridgeway’s intent was all(offset == 0), not (offset == 0).

Original issue reported on code.google.com by chanfe...@gmail.com on 25 Jan 2013 at 6:35

GoogleCodeExporter commented 8 years ago
Thanks for reporting this.

Would this fix it:?

if (all(is.null(offset)) | all(offset == 0)){
  offset <- NA
}

If so, I'll implement it.

Harry

Original comment by harry.southworth on 26 Jan 2013 at 9:53

GoogleCodeExporter commented 8 years ago
Thanks. I think this would fix it.

I see that you added all in front of is.null(offset) and changed || to |. Could 
you explain your rationale? (for my own education)

Original comment by chanfe...@gmail.com on 26 Jan 2013 at 11:47

GoogleCodeExporter commented 8 years ago
[deleted comment]
GoogleCodeExporter commented 8 years ago

Original comment by harry.southworth on 29 Jan 2013 at 11:11

GoogleCodeExporter commented 8 years ago
I'm implemented a version of the fix. I'd be grateful if you were to test it. 
The package version currently hosted on the project home page includes the fix.
Harry

Original comment by harry.southworth on 29 Jan 2013 at 4:54

GoogleCodeExporter commented 8 years ago
I ran a test on gbm 2.0-9.1 and the bug has been fixed. Thank you for your work 
on gbm 2.0. It is great.

Original comment by chanfe...@gmail.com on 30 Jan 2013 at 4:16

GoogleCodeExporter commented 8 years ago

Original comment by harry.southworth on 30 Jan 2013 at 10:28