mean_squared_error with ignore_nan option
#190
Conversation
mottodora
commented
Jun 18, 2018
•
mottodora
commented
- fix test to chainer-chemistry style
- add double backward tests
- add documents
mottodora
changed the title
ignore_nan optionignore_nan option
| """Mean squared error (a.k.a. Euclidean loss) function.""" | ||
|
|
||
| def __init__(self, ignore_nan=False): | ||
| self.task_weight = None |
There was a problem hiding this comment.
Can you write
#TODO (mottodora): implement task weight calculation
so that others can understand it is simply not implemented yet?
There was a problem hiding this comment.
I forgot to delete this line. But I will write comments.
| diff = x0 - x1 | ||
| if self.ignore_nan: | ||
| diff = chainer.functions.where(xp.isnan(diff.array), | ||
| xp.zeros_like(diff.array), diff) |
There was a problem hiding this comment.
I'm not sure but xp.zeros_like(diff.array) can be just 0, in that case we can skip creating (allocating memory of) maybe large zero array.
There was a problem hiding this comment.
chainer where function receive only same shape ndarray. (https://docs.chainer.org/en/stable/reference/generated/chainer.functions.where.html)
| loss_expect += ((x0_data[i] - numpy.nan_to_num(x2_data[i])) ** 2 | ||
| * nan_mask[i]) | ||
| loss_expect /= x0_data.size | ||
| assert numpy.allclose(loss_value, loss_expect) |
There was a problem hiding this comment.
Why did you use numpy.allclose in this function, while pytest.approx in check_forward?
|
|
||
| """Mean squared error (a.k.a. Euclidean loss) function.""" | ||
|
|
||
| def __init__(self, ignore_nan=False): |
There was a problem hiding this comment.
IMO, what "ignore" means can differ from people to people. So, I would suggest more descriptive option name "e.g. convert_nan_to_zero" or write what this option does in detail in the document.
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
==========================================
+ Coverage 76.64% 77.08% +0.44%
==========================================
Files 87 89 +2
Lines 3712 3875 +163
==========================================
+ Hits 2845 2987 +142
- Misses 867 888 +21 |
|
LGTM. |
| return chainer_chemistry.functions.mean_squared_error(x0, x1, | ||
| ignore_nan=True) | ||
| gradient_check.check_backward(func, (x0_data, x2_data), None, eps=1e-2) | ||
|
|
There was a problem hiding this comment.
Could you add tests for backward and double backward with ignore_nan=True using x0 and x1?
