Infinity splits #12
Infinity splits #12
Conversation
…nly one type of bound. Also updated the tests.
…linear) with a single split.
| @@ -62,7 +62,7 @@ plotMargin(1.5) | |||
| out1.5 <- compute_optimal_costs( | |||
| target.mat, margin=1.5, loss="hinge") | |||
| test_that("margin=1.5 yields cost 0,1", { | |||
| expect_equal(out1.5$pred, c(Inf, 1, 0)) | |||
| expect_equal(out1.5$pred, c(0.5, 1, 0)) | |||
There was a problem hiding this comment.
thanks for updating the R unit tests
mmit/tests/test_learner.py
Outdated
|
|
||
| def test_dummy_dataset_1(self): | ||
| """ | ||
| learning on dummy dataset #1 |
There was a problem hiding this comment.
maybe "noiseless" instead of dummy? That would be more specific.
|
another thing I was thinking -- in many data sets, some input features are monotonic transforms of others, e.g. first column is x, second columns is log(x). Do you think that would confuse the learning algo? Do you think we should add a pre-processing step that filters any features that are monotonic transforms of other features? we could do that by comparing the sort order indices of the features... |
…o infinity-splits
| @@ -165,7 +165,7 @@ int PiecewiseFunction::insert_point(double y, bool is_upper_bound) { | |||
| } | |||
|
|
|||
| // Breakpoint info | |||
| float b = y - s * this->margin; | |||
There was a problem hiding this comment.
@tdhock I'm pretty sure this is why we had really poor precision on the breakpoint positions! The breakpoint positions were floats and were getting converted to double each time a comparison was made, since all the other real-valued variables have type double.
With this fix, I was able to increase the tolerance to super small values (e.g., 1e-20) and didn't get any failing tests. For now, I set the tolerance to 1e-9.
There was a problem hiding this comment.
wow, sorry I didn't catch that -- in general I never use float -- that should have raised a red flag in my head -- I completely agree, change to using double all the time.
There was a problem hiding this comment.
although I would not expect that it would make a big difference in terms of test error...
mmit/core/piecewise_function.cpp
Outdated
| * that value + some offset. | ||
| * */ | ||
| return get_breakpoint_position(std::prev(this->min_ptr)); | ||
| return get_breakpoint_position(std::prev(this->min_ptr)) + 1e-4; // Add a small value because the previous breakpoint is not included in the minimum segment ]lower, upper] |
There was a problem hiding this comment.
@tdhock I made this change because b_{t, i-1} is not included in the minimum segment, which is defined over the range ]b_{t, i-1}, b_{t, i}]. Should not change much in practice, but at least it's consistent. I updated the unit tests in R and Python.
|
I would suggest to avoid adding arbitrary constants (like 1e-4) wherever
possible. Is it really necessary? When there are all lower bounds, then the
value of the cost function at the last breakpoint is zero, right? In fact
all predictions in [b, \infty) result in zero cost, right? (where b is the
last breakpoint)
|
|
Yes that’s right. However, as per our definition, b is not part of the
minimum segment, which is ]b, \infty). I added an arbitrary constant
because any value > b is in the minimum segment and leads to zero cost. I
know that arbitrary constants are ugly, but at least this makes the code
consistant with the theory. On the other hand, maybe we could make an
exception to the theory, since we are guaranteed that “b” leads to zero
cost. What’s your take on this: stick to the theory or exception?
|
|
You are right that b is not part of the minimum segment, but we do know
that b is also a minimum. I would recommend to delete the constant, and
just use the last breakpoint. It won't make any difference in terms of
learning, but I think it will clarify the code (it is always better to
avoid introducing arbitrary constants in my opinion).
Another option is to go back to reporting Infty as the minimum (rather than
b).
Another option is to report both the lower and upper bounds of optimal
solutions, here is would be the two double precision numbers b and INFINITY.
On Tue, May 30, 2017 at 1:03 PM, Alexandre Drouin <notifications@github.com>
wrote:
… Yes that’s right. However, as per our definition, b is not part of the
minimum segment, which is ]b, \infty). I added an arbitrary constant
because any value > b is in the minimum segment and leads to zero cost. I
know that arbitrary constants are ugly, but at least this makes the code
consistant with the theory. On the other hand, maybe we could make an
exception to the theory, since we are guaranteed that “b” leads to zero
cost. What’s your take on this: stick to the theory or exception?
On May 30, 2017 at 12:48:58 PM, Toby Dylan Hocking (
***@***.***)
wrote:
I would suggest to avoid adding arbitrary constants (like 1e-4) wherever
possible. Is it really necessary? When there are all lower bounds, then the
value of the cost function at the last breakpoint is zero, right? In fact
all predictions in [b, \infty) result in zero cost, right? (where b is the
last breakpoint)
On Tue, May 30, 2017 at 12:40 PM, Alexandre Drouin <
***@***.***
> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In mmit/core/piecewise_function.cpp
> <#12 (comment)>:
>
> > @@ -101,11 +101,7 @@ double PiecewiseFunction::get_minimum_position()
{
> else if(equal(this->min_coefficients.quadratic, 0) &&
equal(this->min_coefficients.linear, 0)){
> if(is_end(this->min_ptr)){
> // Case: \___x lower bounds only
> - /* TODO: I don't think this is correct. The position of the previous
breakpoint is not in the min
> - * segment. So by returning that value, we are not returning a true
function minimum. We should return
> - * that value + some offset.
> - * */
> - return get_breakpoint_position(std::prev(this->min_ptr));
> + return get_breakpoint_position(std::prev(this->min_ptr)) + 1e-4; //
Add
a small value because the previous breakpoint is not included in the
minimum segment ]lower, upper]
>
> @tdhock <https://github.com/tdhock> I made this change because b_{t,
i-1}
> is not included in the minimum segment, which is defined over the range
> ]b_{t, i-1}, b_{t, i}]. Should not change much in practice, but at least
> it's consistent. I updated the unit tests in R and Python.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#12 (review)>, or
mute
> the thread
> <
https://github.com/notifications/unsubscribe-auth/
AA478urE4OibZruTTk56l4a4KjumANNMks5r_EaIgaJpZM4NA1-M
>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/
ACQ9RKNTIZ6dgpxqFPMkBfVwjwtBY-RMks5r_Eh3gaJpZM4NA1-M>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA478qea7dbqGRJrJf7glAmqN_geJejYks5r_EvKgaJpZM4NA1-M>
.
|
|
Thanks for your opinion! I’ll go with option 1 and roll back to what we
were doing before: return “b” as the predicted value. It’s the simplest
solution and we do know that it is a minimum.
|
… and made a few changes to comments
|
My code review is done and I'm ready to merge into Master. Ok for you @tdhock? |
|
great, please merge +1 |
Update Travis in this branch
Changed the predicted value for the case where the function only contains one type of bound.
Case -- only lower bounds: the predicted value is the position of the first breakpoint to the left of the minimum pointer (i.e, the largest lower bound + margin).
Case -- only upper bounds: the predicted value is the position of the minimum pointer (i.e., the smallest upper bound - margin).
Also updated the unit tests to account for this change.