multi_margin_loss: check weight shape, make contiguous on CPU, add tests
#104852
Conversation
…d tests [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/104852
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New FailureAs of commit 0966cc1: NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
…d tests ghstack-source-id: deae4cd19cdde371632b0f2c00addca770458579 Pull Request resolved: #104852
nkaretnikov
added
ciflow/trunk
|
Summary:
|
| Tensor weight_contiguous; | ||
| if (weight && weight->defined()) { | ||
| weight_contiguous = weight->contiguous(); | ||
| } |
There was a problem hiding this comment.
For compat with multi_margin_loss_cuda_out:
Tensor weights;
if (weights_ && weights_->defined()) {
weights = weights_->contiguous();
}
| @@ -103,15 +103,15 @@ void multi_margin_loss_out_cpu_template( | |||
| const Tensor& target, | |||
| int p, | |||
| const Scalar& margin, | |||
| const Tensor& weight, | |||
| const c10::optional<Tensor>& weight, | |||
There was a problem hiding this comment.
multi_margin_loss_shape_check accepts an optional Tensor because weight is an optional on CUDA. So I change this (and elsewhere) to be consistent.
| // See [Note: hacky wrapper removal for optional tensor] | ||
| c10::MaybeOwned<Tensor> weight_maybe_owned = at::borrow_from_optional_tensor(weight_opt); | ||
| const Tensor& weight = *weight_maybe_owned; | ||
|
|
There was a problem hiding this comment.
Here and elsewhere: to have a consistent API for multi_margin_loss_shape_check (it accepts optional weight on CPU and CUDA).
| weight->dim() <= 1 && weight->numel() == dim, | ||
| "inconsistent weight size, expected ", dim, " but got ", | ||
| weight->sizes()); | ||
| } |
There was a problem hiding this comment.
From the docs: "a manual rescaling weight given to each class. If given, it has to be a Tensor of size C. Otherwise, it is treated as if having all ones." But target here can be a scalar, so could it be useful to allow scalars for weight?
https://pytorch.org/docs/stable/generated/torch.nn.MultiMarginLoss.html
| @@ -1405,6 +1406,7 @@ def sample_inputs_multi_margin_loss(op_info, device, dtype, requires_grad, **kwa | |||
| ((S, M), make_target([S], low=0, high=M), {"margin": 1.0}), | |||
| ((S, M), make_target([S], low=0, high=M), {"margin": -3.14}), | |||
| ((M, S), make_target([M], low=0, high=S), {"weight": None}), | |||
| ((M, S), make_target([M], low=0, high=S), {"weight": make_weight([S], low=-10., high=10.)}), | |||
There was a problem hiding this comment.
weight wasn't tested before at all.
| for (input_shape, target), p, margin, reduction in product(inputs, ps, margins, reductions): | ||
| kwargs = {"p": p, "margin": margin} | ||
| for (input_shape, target), p, margin, weight, reduction in product(inputs, ps, margins, weights, reductions): | ||
| kwargs = {"p": p, "margin": margin, "weight": weight} |
There was a problem hiding this comment.
More tests with different combinations of arguments.
| yield ErrorInput(SampleInput(make_input(5, 4), args=(make_input(5,),), kwargs={'weight': make_input(())}), | ||
| error_type=ValueError, error_regex='weight must be one-dimensional') | ||
| yield ErrorInput(SampleInput(make_input(5, 4), args=(make_input(5,),), kwargs={'weight': make_input(5, 4)}), | ||
| error_type=ValueError, error_regex='weight must be one-dimensional') |
There was a problem hiding this comment.
This "one-dimensional" error is only on the Python side. See my comment above, maybe worth removing this restriction from the Python side and allow scalars?
| yield ErrorInput(SampleInput(make_input(5, 4), args=(make_input(5,),), kwargs={'weight': make_input(5, 4)}), | ||
| error_type=ValueError, error_regex='weight must be one-dimensional') | ||
| yield ErrorInput(SampleInput(make_input(5, 4), args=(make_input(5,),), kwargs={'weight': make_input(5,)}), | ||
| error_type=RuntimeError, error_regex=r'inconsistent weight size, expected 4 but got \[5\]') |
There was a problem hiding this comment.
Raises an error now.
… on CPU, add tests" [ghstack-poisoned]
…d tests ghstack-source-id: 585ed2c4981b8327d1ad1301c896cf8f8a63c0ea Pull Request resolved: #104852
| "TestJit", | ||
| "test_variant_consistency_jit", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Tests now multiply by weight, which affects accuracy here.
nkaretnikov
removed
the
ciflow/periodic
… on CPU, add tests" [ghstack-poisoned]
… on CPU, add tests" [ghstack-poisoned]
Stack from ghstack:
multi_margin_lossops #104578multi_margin_loss: checkweightshape, make contiguous on CPU, add tests #104852multi_margin_loss_shape_checkon CPU and CUDA #104851multi_margin_loss#104850